FYI. There are many places in the compiler where it uses an 'int' as a boolean flag so this is a slippery slope.
My personal preference is to define flags like this using
int name1:1;
int name2:1;
int name3:1;
See common.h for real examples of using this style of boolean flag.
Then you can code it as name1 = TRUE; or name1 = 1; and test via if(name1) { do something} etc....
But the GnuCOBOL compiler had been using 'int' and I did not want to stray away from that approach.
After reportwriter is 100% merged into trunk that is when a lot of things could be cleaned up but I've been waiting for a long time for that to happen.
Changing from 'int' to 'int xxx:1' would only require a change to the header file and NO code changes at all....
You have to be very careful as make such changes could result in forcing a recompile of all the COBOL application code with a new release of the compiler.
I think a recompile is not a big deal but others get annoyed about it...
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
