Re: simplehash.h comment

On Mon, Aug 20, 2018 at 4:57 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Aug 20, 2018 at 04:38:20PM +1200, Thomas Munro wrote:
> > "For examples of usage look at simplehash.c ..."
> >
> > There is no such file in the tree.  Maybe this should say tidbitmap.c?
>
> And execGrouping.c...

Some more comments on simplehash.h:

It fails to undefine SH_EQUAL where it undefines the other 'parameter' macros.

The static function definitions sh_log2 and sh_pow2 need to be wrapped
in include guards, or moved to a different header (or merged with one
of our other definitions of those function), otherwise you can't use
it more than once per translation unit.

#define SH_STATUS_EMPTY SH_MAKE_NAME(EMPTY)'s use of EMPTY is a bit of
a macro-collision hazard.  I managed to collide with the EMPTY macro
defined in regcomp.c.  (That may indicate that my header is getting
included a little too generally, admittedly, but that's beside the
point.)

Just a thought:  It seems plausible to let the caller provide a way
for 'used' and 'empty' to be encoded, so that you don't have to
provide a struct with a member 'status'.  For example if your struct
has an Oid member, or the entry type itself is (say) an Oid instead of
a struct, then it might be possible to use InvalidObjectId to indicate
an unused slot.

I wouldn't have called a hash table a "hash".  (#define SH_TYPE
SH_MAKE_NAME(hash)).  A hash is a hash, a hash table is a hash table
(unless you were overexposed to Perl as a child :-D).

-- 
Thomas Munro
http://www.enterprisedb.com