Thread: simplehash.h comment

Hi,

"For examples of usage look at simplehash.c ..."

There is no such file in the tree.  Maybe this should say tidbitmap.c?

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

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..
--
Michael

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

On Sun, Aug 26, 2018 at 2:41 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Some more comments on simplehash.h:

... and a patch.  Thoughts?

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

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> ... and a patch.  Thoughts?

It's not real obvious why you're adding inclusion guards over just a
subset of the header.  A short comment about that would be worthwhile.

            regards, tom lane

On Tue, Aug 28, 2018 at 1:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
> > ... and a patch.  Thoughts?
>
> It's not real obvious why you're adding inclusion guards over just a
> subset of the header.  A short comment about that would be worthwhile.

Right, thanks.  I added a comment and committed.

Obviously the longer term fix for that particular issue is not to
define those functions in that header at all.   If no one beats me to
it, I'd like to have another look at consolidating all our popcount()
and ffs() functions[1], and this fls()/log2(), pow2() stuff seems like
it might be part of the same refactoring expedition.  For now I just
added the missing include guards to unblock some other stuff I'm about
to propose that wants to use simplehash.h.

[1]
https://www.postgresql.org/message-id/flat/CAEepm%3D3k%2B%2BYtf2LNQCvpP6m1%3DgY9zZHP_cfnn47%3DWTsoCrLCvA%40mail.gmail.com

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