Re: define bool in pgtypeslib_extern.h

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> I'm wondering whether we should actually go the opposite way and say
> that c.h's "bool" definition should be backend only, and that in
> frontend code we should define a PG_bool type or something of that ilk
> for when we want "PG's 1-byte bool" and otherwise let the platform
> define "bool" however it wants.
> And we certainly shouldn't be defining "bool" in something that's going
> to be included in the user's code the way that ecpglib.h is.

I experimented with doing things that way, and ended up with the attached
draft patch. It basically gets ecpglib.h out of the business of declaring
any bool-related stuff at all, instead insisting that client code include
<stdbool.h> or otherwise declare bool for itself. The function
declarations that were previously relying on "bool" now use the "pqbool"
typedef that libpq-fe.h was already exporting. Per discussion, that's
not an ABI break, even on platforms where sizeof(_Bool) > 1, because
the actual underlying library functions are certainly expecting to take
or return a value of size 1.

While this seems like a generally cleaner place to be, I'm a bit concerned
about a number of aspects:

* This will of course be an API break for clients, which might not've
included <stdbool.h> before.

* On platforms where sizeof(_Bool) > 1, it's far from clear to me that
ECPG will interface correctly with client code that is treating bool
as _Bool. There are some places that seem to be prepared for bool
client variables to be either sizeof(char) or sizeof(int), for example
ecpg_store_input(), but there are a fair number of other places that
seem to assume that sizeof(bool) is relevant, which it won't be.
The ECPG regression tests do pass for me on a PPC Mac, but I wonder
how much that proves.

* The "sql/dyntest.pgc" test declares BOOLVAR as "char" and then does

exec sql var BOOLVAR is bool;

It's not clear to me what the implications of that statement are
(and our manual is no help), but looking at the generated code,
it seems like this causes ecpg to believe that the size of the
variable is sizeof(bool). So that looks like buffer overrun
trouble waiting to happen. I changed the variable declaration to
"bool" in the attached, but I wonder what's supposed to be getting
tested there.

On the whole I'm not finding this an attractive way to proceed
compared to the other approach I sketched. It will certainly
cause some clients to have compile failures, and I'm at best
queasy about whether it will really work on platforms where
sizeof(_Bool) > 1. I think we're better off to go with the
other approach of making ecpglib.h export what we think the
correct definition of bool is. For most people that will
end up being <stdbool.h>, which I think will be unsurprising.

regards, tom lane

PS: another issue this fixes, which I think we ought to fix and back-patch
regardless of what we decide about bool, is it moves the declaration for
ecpg_gettext() out of ecpglib.h and into the private header
ecpglib_extern.h. That function isn't meant for client access, the
declaration is wrong where it is because it is not inside extern "C",
and the declaration wouldn't even compile for clients because they
will not know what pg_attribute_format_arg() is. The only reason we've
not had complaints, I imagine, is that nobody's tried to compile client
code with ENABLE_NLS defined ... but that's already an intrusion on
client namespace.