On Tue, Dec 26, 2017 at 02:00:47PM -0500, Peter Eisentraut wrote:
> On 12/25/17 00:32, Michael Paquier wrote:
> >> So here is a minimal patch set to perhaps wrap this up for the time
> >> being. I have added static assertions that check the sizes of
> >> GinNullCategory and GinTernaryValue, which I think are the two critical
> >> places that require compatibility with bool. And then we include
> >> <stdbool.h> only if its bool has size 1.
> >
> > + /*
> > + * now we can use the nullFlags as category codes
> > + */
> > + StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
> > + "sizes of GinNullCategory and bool are not equal");
> > *categories = (GinNullCategory *) nullFlags;
> >
> > Hm. So on powerpc compilation is going to fail with this patch as
> > sizeof(char) is 1, no?
>
> Yes, but right now it would (probably) just fail in mysterious ways, so
> the static assertion adds safety.
>
> > Wouldn't it be better to just allocate an array
> > for GinNullCategory entries and then just fill in the values one by
> > one with GIN_CAT_NORM_KEY or GIN_CAT_NULL_KEY by scanning nullFlags?
>
> Yeah, initially I though making another loop through the array would be
> adding more overhead. But reading the code again, we already loop
> through the array anyway to set the nullFlags to the right bit patterns.
> So I think we can just drop that and build a proper GinNullCategory
> array instead. I think that would be much cleaner. Patch attached.
Thanks for the updated patch. This looks saner to me.
It would be nice to do something like that for GinTernaryValue in
tsginidx.c by mapping directly to GIN_FALSE and GIN_TRUE depending on
the input coming in gin_tsquery_consistent. The fix is more trivial
there. Do you think that those two things should be back-patched?
--
Michael
