Re: [GnuCOBOL-users] flags in cob_file_key

[Ron Norman]

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 <address@hidden> 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