I would recommend that you leave this as is until the reportwriter code is fully merged into trunk as making such changes now will make even more work for Simon and delay the final merge even more.
On Wed, Feb 20, 2019 at 4:34 PM James K. Lowden <jklowden@xxxxxxxxxxxxxxx> wrote:
Looking at changes in the 3.0 branch to cob_file_key in
libcob/common.h (from 2.0), I see a move to discrete flags for whether
the key allows duplicates, is ascending, and if missing values are
suppressed. While I applaud the intention, I would like to suggest a
more efficient scheme that would be easier to use.
The current struct looks, in part, like this:
typedef struct __cob_file_key {
...
int tf_duplicates;
int tf_ascending;
int tf_suppress;
...
} cob_file_key;
That's 3 * 8 = 24 bytes on a 64-bit architecture for three boolean
values, when only 3 bits are needed.
I suggest instead enumerated values for the flags, and functions to
test and set them.
enum cob_file_option_t {
CF_DUPLICATES = 1,
CF_ASCENDING = 2,
CF_SUPPRESS = 4,
CF_ALL_FLAGS = 7
};
typedef struct __cob_file_key {
...
int options;
...
} cob_file_key;
static inline bool
duplicate_keys( const cob_file_key *key ) {
return (key->options & CF_DUPLICATES) > 0;
}
static inline bool
ascending_keys( const cob_file_key *key ) {
return (key->options & CF_ASCENDING) > 0;
}
static inline bool
suppressed_keys( const cob_file_key *key ) {
return (key->options & CF_SUPPRESS) > 0;
}
static inline int
cob_file_key_option_set( cob_file_key *key,
enum cob_file_option_t option )
{
return (key->options |= option);
}
static inline int
cob_file_key_option_clear( cob_file_key *key,
enum cob_file_option_t option )
{
return (key->options &= ~option);
}
Here I'm taking advantage of a 20-year-old standard, C99, which
introduced static inline functions and stdbool.h.
IMO the code becomes clearer by use of the functions. For example:
if( key->tf_duplicates == 0 )
versus
if( !duplicate_keys(key) )
The places where the key is being set/reset is made easier to locate in
the code by the functions. Instead of looking for a variable
assignment, we look for function invocation:
key->tf_duplicates = 1;
versus
cob_file_key_option_set( key, CF_DUPLICATES);
If you feel these names are too long, I might agree. I could also be
happy with cfk_set and cfk_clear if those were deemed acceptable. In
fact, if it's not too fancy, I'd suggest PROP_SET and PROP_CLEAR macros,
and use _Generic (from C11) to discriminate underlying functions by
argument type (a bit like function overloading).
I would like to switch to this system of key properties as part of our
work on the LMDB ISAM implementation. I wanted to get a sense of the
senate first.
--jkl
Cheers
Ron Norman
