Re: GinPageIs* don't actually return a boolean

On 2016-03-02 21:48:16 -0500, Peter Eisentraut wrote:
> After reviewing this thread and relevant internet lore, I think this
> might be the wrong way to address this problem. It is in general not
> guaranteed in C that a Boolean-sounding function or macro returns 0 or
> 1. Prime examples are ctype.h functions such as isupper(). This is
> normally not a problem because built-in conditionals such as if, &&, ||
> handle this just fine. So code like
>
> - Assert(!create || !!txn);
> + Assert(!create || txn != NULL);
>
> is arguably silly either way. There is no risk in writing just
>
> Assert(!create || txn);

Yes, obviously. I doubt that anyone misunderstood that. I personally
find the !! easier to read when contrasting to a negated value (as in
the above assert).

> The problem only happens if you compare two "Boolean" values directly
> with each other;

That's not correct. It also happen if you compare an expression with a
stored version, i.e. only one side is a 'proper bool'.

> A quick look through the code based on the provided patch shows that
> approximately the only place affected by this is
>
> if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
> elog(ERROR, "right sibling of GIN page is of different type");
>
> and that's not actually a problem because isLeaf and isData are earlier
> populated by the same macros. It would still be worth fixing, but a
> localized fix seems better.

That's maybe the only place where we compare stored booleans to
expressions, but it's definitely not the only place where some
expression is stored in a boolean variable. And in all those cases
there's an int32 (or whatever type the expression has) to bool (usually
1byte char). That's definitely problematic.

> But the lore on the internet casts some doubt on that: There is no
> guarantee that bool is 1 byte, that bool can be passed around like char,
> or even that bool arrays are laid out like char arrays. Maybe this all
> works out okay, just like it has worked out so far that int is 4 bytes,
> but we don't know enough about it. We could probably add some configure
> tests around that.

So?

> My proposal on this particular patch is to do nothing. The stdbool
> issues should be looked into, for the sake of Windows and other
> future-proofness. But that will likely be an entirely different patch.

That seems to entirely miss the part of this discussion dealing with
high bits set and such?

Greetings,

Andres Freund