taking stdbool.h into use

On Wed, Aug 16, 2017 at 4:36 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Some not so long time ago, it was discussed to look into taking
stdbool.h into use. The reason was that third-party libraries (perl?,
ldap?, postgis?) are increasingly doing so, and having incompatible
definitions of bool could/does create a mess.

Here is a patch set that aims to accomplish that. On the way there, it
cleans up various loose and weird uses of bool and proposes a way to
ensure that the system catalog structs get a 1-byte bool even if the
"standard" bool is not.

I have done a fair amount of testing on this, including a hand-rigged
setup where _Bool is not 1 byte. But obviously, more and wider testing
would be very useful.

Getting out of the way of C99 "bool" makes sense. It is old enough to
vote. On my system the tests pass at top level and tcl, plperl,
plpython after each patch is applied, when configured with:

--enable-debug --enable-depend --enable-cassert --with-icu --with-python
--with-perl --with-tcl --with-icu --with-ldap

However my system has sizeof(bool) == 1 and so do all the systems I
have access to (x86 + POWER). Where can we find a computer with
sizeof(bool) == 4? According to the intertubes OSX on POWER and
Windows 32 bit systems had that in ancient prehistory but they don't
now.

0001-Fix-bool-int-type-confusion.patch

Looks good.

0002-Change-TRUE-FALSE-to-true-false.patch

Looks good. What about these?

src/backend/port/win32/crashdump.c: ExInfo.ClientPointers = FALSE;
src/backend/port/win32/signal.c: pgwin32_signal_event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32/signal.c:
SleepEx(500, FALSE);
src/backend/port/win32/signal.c: return FALSE;
src/backend/port/win32/socket.c: waitevent =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32/socket.c: r =
WaitForMultipleObjectsEx(2, events, FALSE, 100, TRUE);
src/backend/port/win32/socket.c: r =
WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE);
src/backend/port/win32/socket.c: r =
WaitForMultipleObjectsEx(numevents + 1, events, FALSE, timeoutval,
TRUE);
src/backend/port/win32/timer.c: r =
WaitForSingleObjectEx(timerCommArea.event, waittime, FALSE);
src/backend/port/win32/timer.c: timerCommArea.event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32_sema.c: rc =
WaitForMultipleObjectsEx(2, wh, FALSE, INFINITE, TRUE);
src/backend/port/win32_shmem.c: hmap = OpenFileMapping(FILE_MAP_READ,
FALSE, szShareMem);
src/backend/storage/ipc/dsm_impl.c:
FALSE, /* do not inherit the name */
src/backend/storage/ipc/dsm_impl.c:
PostmasterHandle, &hmap, 0, FALSE,
src/backend/storage/ipc/dsm_impl.c:
NULL, NULL, 0, FALSE,
src/backend/storage/ipc/latch.c: latch->event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/storage/ipc/latch.c: latch->event =
CreateEvent(&sa, TRUE, FALSE, NULL);
src/interfaces/ecpg/ecpglib/misc.c: return FALSE;
src/interfaces/ecpg/ecpglib/misc.c: return FALSE;
src/interfaces/ecpg/ecpglib/misc.c: mutex->handle
= CreateMutex(NULL, FALSE, NULL);
src/interfaces/ecpg/pgtypeslib/datetime.c: bool
EuroDates = FALSE;
src/interfaces/ecpg/pgtypeslib/datetime.c: bool
EuroDates = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: int
bc = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: int
is2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: int
haveTextMonth = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: int
is2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: int
bc = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c: bool
is_before = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c: *is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c: *is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c: bool
is_before = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:
is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/numeric.c: bool have_dp = FALSE;
src/interfaces/ecpg/pgtypeslib/timestamp.c: return FALSE;
src/interfaces/libpq/fe-secure.c: * The caller should say got_epipe =
FALSE if it is certain that it
src/pl/plperl/plperl.c:
eval_pv("PostgreSQL::InServer::SPI::bootstrap()", FALSE);
src/pl/plperl/plperl.c: eval_pv(PLC_TRUSTED, FALSE);
src/pl/plperl/plperl.c: eval_pv("my $a=chr(0x100); return $a =~
/\\xa9/i", FALSE);
src/pl/plperl/plperl.c: eval_pv(plperl_on_plperl_init, FALSE);
src/pl/plperl/plperl.c: eval_pv(plperl_on_plperlu_init, FALSE);
src/pl/plperl/plperl.c: SV **svp = av_fetch(av,
i, FALSE);
src/pl/plperl/plperl.c: while ((svp = av_fetch(rav, i,
FALSE)) != NULL)
src/pl/plperl/ppport.h:# define ERRSV
get_sv("@",FALSE)
src/pl/plperl/ppport.h: utilize(!(flags & PERL_LOADMOD_DENY),
start_subparse(FALSE, 0),
src/pl/plperl/ppport.h: start_subparse(FALSE, 0),
src/pl/plperl/ppport.h: SV *my_cxt_sv = get_sv(MY_CXT_KEY, FALSE)
src/pl/plperl/ppport.h: return FALSE;
src/pl/plperl/ppport.h: bool overflowed = FALSE;
src/pl/plperl/ppport.h: bool overflowed = FALSE;
src/pl/plperl/ppport.h: bool overflowed = FALSE;
src/pl/plpgsql/src/pl_comp.c: * if not recognized, fill in *word and
return FALSE.
src/port/kill.c: if ((prochandle =
OpenProcess(PROCESS_TERMINATE, FALSE, (DWORD) pid)) == NULL)
src/port/pgsleep.c: SleepEx((microsec < 500 ? 1 :
(microsec + 500) / 1000), FALSE);
src/test/regress/pg_regress.c: r =
WaitForMultipleObjects(tests_left, active_pids, FALSE, INFINITE);

0003-Remove-TRUE-and-FALSE.patch

Looks good. What about these?

src/interfaces/ecpg/include/ecpglib.h:#define FALSE 0
src/interfaces/ecpg/pgtypeslib/extern.h:#define FALSE 0

0004-Remove-BoolPtr-type.patch

Looks good.

0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch

- * For convenience, this is compatible with booleans. A boolean can be
+ * For convenience, this is compatible with bools. A bool can be

Maybe "compatible with bool" ?

+#elif SIZEOF_BOOL == 4
+typedef int GinTernaryValue;
+#else

Maybe int32? I understand that PostgreSQL doesn't actually work if
int isn't of that size, but still.

0006-Add-bool8-typedef-for-system-catalog-structs.patch

Make sense.

0007-Avoid-use-of-bool-in-thread_test.c.patch

Looks good.

0008-Use-stdbool.h-if-available.patch

I'm afraid this autoconf stuff is gibberish, and I can't personally
tell if it's the right gibberish so no opinion on this one.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thomas Munro <thomas.munro@enterprisedb.com> writes:

However my system has sizeof(bool) == 1 and so do all the systems I
have access to (x86 + POWER). Where can we find a computer with
sizeof(bool) == 4? According to the intertubes OSX on POWER and
Windows 32 bit systems had that in ancient prehistory but they don't
now.

prairiedog and (I assume) locust. Maybe coypu --- I imagine OSX
got that choice from upstream BSD someplace.

cube:~ tgl$ uname -a
Darwin cube.sss.pgh.pa.us 8.11.0 Darwin Kernel Version 8.11.0: Wed Oct 10 18:26:00 PDT 2007; root:xnu-792.24.17~1/RELEASE_PPC Power Macintosh powerpc
cube:~ tgl$ cat foo.c
#include <stdio.h>
#include <stdbool.h>

int main()
{
printf("sizeof(bool) = %zu\n", sizeof(bool));
return 0;
}
cube:~ tgl$ gcc -O -Wall foo.c
cube:~ tgl$ ./a.out
sizeof(bool) = 4

Don't know how far back you need to go to find Windows machines
with 4-byte bool, but we have some pretty long-in-the-tooth
buildfarm critters in that lineage, too.

gaur/pademelon isn't booted up right now, but it might provide
an example of a system that lacks <stdbool.h> altogether.
(If it doesn't, I'd be willing to concede that we need not
consider that scenario anymore.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

gaur/pademelon isn't booted up right now, but it might provide
an example of a system that lacks <stdbool.h> altogether.
(If it doesn't, I'd be willing to concede that we need not
consider that scenario anymore.)

For the record --- pademelon (vendor cc on that box) doesn't have
<stdbool.h> at all. gaur (user-installed gcc) has such a header,
but it contains

typedef enum
{
false = 0,
true = 1
} bool;

which unsurprisingly results in
sizeof(bool) = 4

What's possibly more relevant to Peter's patch, this represents
a platform on which "#include <stdbool.h>" succeeds, but
(a) there is no typedef _Bool, and (b) "bool" is not a macro.
Obviously pre-C99 ...

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Aug 16, 2017 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Don't know how far back you need to go to find Windows machines
with 4-byte bool, but we have some pretty long-in-the-tooth
buildfarm critters in that lineage, too.

From VS 2003 and upwards the size has always been 1:
https://msdn.microsoft.com/en-us/library/s3f49ktz.aspx
So this gives some margin as the buildfarm uses VS 2008 and newer versions.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

Some not so long time ago, it was discussed to look into taking
stdbool.h into use. The reason was that third-party libraries (perl?,
ldap?, postgis?) are increasingly doing so, and having incompatible
definitions of bool could/does create a mess.

Here is a patch set that aims to accomplish that. On the way there, it
cleans up various loose and weird uses of bool and proposes a way to
ensure that the system catalog structs get a 1-byte bool even if the
"standard" bool is not.

I played around with this on my dinosaur machines.

On gaur, every source file gives me a complaint like this:

In file included from ../../src/include/postgres.h:47,
from username.c:16:
../../src/include/c.h:207: warning: `SIZEOF__BOOL' redefined
../../src/include/pg_config.h:801: warning: this is the location of the previous definition

pg_config.h contains:

/* The size of `_Bool', as computed by sizeof. */
#define SIZEOF__BOOL 0

c.h then attempts to override that, which would be bad style in any case.
I think you should make configure take care of such situations and emit
the correct SIZEOF__BOOL value to begin with. Perhaps the script could
check for a zero result and change it to 1.

Note that even though this platform has a <stdbool.h>, configure rejects
it because the name "bool" is not a macro. Dunno if we care to change
that; I see we're using a standard autoconf macro, so messing with that
is likely more trouble than it's worth.

After temporarily commenting out the bogus SIZEOF__BOOL definition,
I got a clean compile and clean regression tests. That's not too
surprising though because without use of <stdbool.h> it's effectively
equivalent to the old code.

BTW, I also wonder why 0008 is doing

AC_CHECK_SIZEOF(_Bool)
and then
#define SIZEOF_BOOL SIZEOF__BOOL

rather than just

AC_CHECK_SIZEOF(bool)

I should think that not touching _Bool when we don't have to is a
good thing.

On prairiedog, configure seems to detect things correctly:

$ grep BOOL src/include/pg_config.h
#define HAVE_STDBOOL_H 1
#define HAVE__BOOL 1
#define SIZEOF__BOOL 4

It builds without warnings, but the regression tests crash:

2017-08-24 16:53:42.621 EDT [24029] LOG: server process (PID 24311) was terminated by signal 10: Bus error
2017-08-24 16:53:42.621 EDT [24029] DETAIL: Failed process was running: CREATE INDEX textarrayidx ON array_index_op_test USING gin (t);

The core dump is a bit confused, but it seems to be trying to dereference
a null pointer in bttextcmp. I'm pretty sure the underlying problem is
that you've not done anything with GinNullCategory:

/*
* Category codes to distinguish placeholder nulls from ordinary NULL keys.
* Note that the datatype size and the first two code values are chosen to be
* compatible with the usual usage of bool isNull flags.
*
* GIN_CAT_EMPTY_QUERY is never stored in the index; and notice that it is
* chosen to sort before not after regular key values.
*/
typedef signed char GinNullCategory;

Overlaying that with "bool" is just Not Gonna Work. It also ain't gonna
work to do "typedef bool GinNullCategory", so I'm not very sure how to
resolve that. Maybe we could hack it like

#if SIZEOF__BOOL == 1
typedef signed char GinNullCategory;
#elif SIZEOF__BOOL == 4
typedef int32 GinNullCategory;
#else
#error "unsupported sizeof(bool)"
#endif

However, the quoted comment implies that we store GinNullCategory values
on-disk, which might mean that some additional hacks are needed to
preserve index compatibility.

I have done a fair amount of testing on this, including a hand-rigged
setup where _Bool is not 1 byte.

I'm not very sure how you got through regression tests despite this issue.
Possibly it's got something to do with prairiedog being an alignment-picky
machine ... but the bus error is *not* at a spot where a bool or
GinNullCategory value is being accessed, so the problem seems like it
should manifest with jury-rigged _Bool on non-picky hardware as well.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8/16/17 02:32, Thomas Munro wrote:

0001-Fix-bool-int-type-confusion.patch

Looks good.

Committed that one.

0002-Change-TRUE-FALSE-to-true-false.patch

Looks good. What about these?

OK, did more digging. Windows, ICU, and Perl define their own
TRUE/FALSE, so in those uses we don't have to make any changes. ECPG
also has its own definitions. In ECPG I also found some obsolete uses
for bools that have been fixed in the equivalent backend code a long
time ago, so I added patches to fix that. And another use in
src/backend/port/dynloader/darwin.c was probably wrong to begin with as
well.

I also went through all the comments to make equivalent changes. I kept
that in a separate patch for easier viewing, but it should probably be
committed together.

Using a case-insensitive diff mechanism one can verify that no logic was
changed here.

0003-Remove-TRUE-and-FALSE.patch

Looks good. What about these?

src/interfaces/ecpg/include/ecpglib.h:#define FALSE 0
src/interfaces/ecpg/pgtypeslib/extern.h:#define FALSE 0

Not sure about that. These are available for use by ecpg applications,
and it's perhaps not worth disrupting that.

0004-Remove-BoolPtr-type.patch

Looks good.

Committed that one.

0007-Avoid-use-of-bool-in-thread_test.c.patch

Looks good.

and that one

0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
0008-Use-stdbool.h-if-available.patch

These need some more work based on Tom's feedback.

Attached is a new patch set. Based on the discussion so far, 0001
through 0007 might be ready; the other two need some more work.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
0008-Use-stdbool.h-if-available.patch

These need some more work based on Tom's feedback.

Attached is a new patch set. Based on the discussion so far, 0001
through 0007 might be ready; the other two need some more work.

Do you need me to do another round of tests on prairiedog/gaur/pademelon?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9/14/17 22:35, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
0008-Use-stdbool.h-if-available.patch

These need some more work based on Tom's feedback.

Attached is a new patch set. Based on the discussion so far, 0001
through 0007 might be ready; the other two need some more work.

Do you need me to do another round of tests on prairiedog/gaur/pademelon?

No, I haven't done any further work on those portability-critical pieces
yet.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Here is an updated patch set. This is just a rebase of the previous
set, no substantial changes. Based on the discussion so far, I'm
proposing that 0001 through 0007 could be ready to commit after review,
whereas the remaining two need more work at some later time.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut wrote:

Here is an updated patch set. This is just a rebase of the previous
set, no substantial changes. Based on the discussion so far, I'm
proposing that 0001 through 0007 could be ready to commit after review,
whereas the remaining two need more work at some later time.

I gave this a quick run, to see if my compiler would complain for things
like this:

bool isprimary = flags & INDEX_CREATE_IS_PRIMARY;

(taken from the first patch at
/messages/by-id/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )

which is assigning a value other than 1/0 to a bool variable without an
explicit cast. I thought it would provoke a warning, but it does not.
Is that expected? Is my compiler too old/new?

config.log says
gcc version 6.3.0 20170516 (Debian 6.3.0-18)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

I gave this a quick run, to see if my compiler would complain for things
like this:

bool isprimary = flags & INDEX_CREATE_IS_PRIMARY;

(taken from the first patch at
/messages/by-id/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )

which is assigning a value other than 1/0 to a bool variable without an
explicit cast. I thought it would provoke a warning, but it does not.
Is that expected? Is my compiler too old/new?

It seems to me that this proves the point of the proposed patch. You
had better use a zero-equality comparison for such bitwise operation,
and so you ought to do that:
bool isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier wrote:

On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

I gave this a quick run, to see if my compiler would complain for things
like this:

bool isprimary = flags & INDEX_CREATE_IS_PRIMARY;

(taken from the first patch at
/messages/by-id/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )

which is assigning a value other than 1/0 to a bool variable without an
explicit cast. I thought it would provoke a warning, but it does not.
Is that expected? Is my compiler too old/new?

It seems to me that this proves the point of the proposed patch. You
had better use a zero-equality comparison for such bitwise operation,
and so you ought to do that:
bool isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;

Right, exactly. But my point is that with the whole patch series
applied I didn't get any warnings.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 26, 2017 at 3:48 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Right, exactly. But my point is that with the whole patch series
applied I didn't get any warnings.

Sorry, I misread your message. You use Linux I suppose, what's your compiler?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Michael Paquier wrote:

It seems to me that this proves the point of the proposed patch. You
had better use a zero-equality comparison for such bitwise operation,
and so you ought to do that:
bool isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;

Right, exactly. But my point is that with the whole patch series
applied I didn't get any warnings.

While warnings for this would be lovely, I don't see how we can expect to
get any. This is perfectly correct C code no matter whether isprimary
is C99 bool or is typedef'd to char ... you just end up with different
values of isprimary, should the RHS produce something other than 1/0.
The compiler has no way to know that assigning, say, 4 in the char
variable case is not quite your intent. Maybe you could hope for a
warning if the bit value were far enough left to actually not fit into
"char", but otherwise there's nothing wrong.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 26, 2017 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

While warnings for this would be lovely, I don't see how we can expect to
get any. This is perfectly correct C code no matter whether isprimary
is C99 bool or is typedef'd to char ... you just end up with different
values of isprimary, should the RHS produce something other than 1/0.
The compiler has no way to know that assigning, say, 4 in the char
variable case is not quite your intent. Maybe you could hope for a
warning if the bit value were far enough left to actually not fit into
"char", but otherwise there's nothing wrong.

This reminded me of
/messages/by-id/20160212144735.7zkg5527i3un3254@alap3.anarazel.de
which has caused commit af4472bc when using stdbool.h for MSVC
2013/2015 builds. So I would really assume that there are places where
we could see warnings.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is an updated patch set. This is just a rebase of the previous
set, no substantial changes. Based on the discussion so far, I'm
proposing that 0001 through 0007 could be ready to commit after review,
whereas the remaining two need more work at some later time.

I had a look at this patch series. Patches 1, 2 (macos headers indeed
show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
to me.

I spotted a couple of other things while looking at your patches and
the code tree.

-   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
+   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
You could simplify that at the same time by removing such things. The
"false : true" pattern is less frequent than the "true : false"
pattern.

Some comments in the code still mention FALSE or TRUE:
- hashsearch.c uses FALSE in some comments.
- In execExpr.c, ExecCheck mentions TRUE.
- AggStateIsShared mentions TRUE and FALSE.
- In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

0009 looks like a good idea. It would make the code less confusing for
the reader to mention HAVE_STDBOOL_H in the #endif of c.h that you are
complicating to make the end of the block clear. I am lacking energy
for 0008, so I have no opinions to offer. Sorry :p
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/29/17 08:50, Michael Paquier wrote:

On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is an updated patch set. This is just a rebase of the previous
set, no substantial changes. Based on the discussion so far, I'm
proposing that 0001 through 0007 could be ready to commit after review,
whereas the remaining two need more work at some later time.

I had a look at this patch series. Patches 1, 2 (macos headers indeed
show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
to me.

Committed 1, 2, 3; will work on the rest later and incorporate your
findings.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/29/17 08:50, Michael Paquier wrote:

I had a look at this patch series. Patches 1, 2 (macos headers indeed
show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
to me.

Committed 4 and 5 together.

I spotted a couple of other things while looking at your patches and
the code tree.

-   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
+   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
You could simplify that at the same time by removing such things. The
"false : true" pattern is less frequent than the "true : false"
pattern.

I have found many more instances like that. It might be worth
simplifying a bit, but that sounds like a separate undertaking.

Some comments in the code still mention FALSE or TRUE:
- hashsearch.c uses FALSE in some comments.
- In execExpr.c, ExecCheck mentions TRUE.

That one is an SQL TRUE, so I left it.

- AggStateIsShared mentions TRUE and FALSE.
- In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

Fixed the other ones.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 9, 2017 at 1:46 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 10/29/17 08:50, Michael Paquier wrote:

I spotted a couple of other things while looking at your patches and
the code tree.

-   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
+   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
You could simplify that at the same time by removing such things. The
"false : true" pattern is less frequent than the "true : false"
pattern.

I have found many more instances like that. It might be worth
simplifying a bit, but that sounds like a separate undertaking.

Yeah, I just mentioned one for reference. And I can see 66 instances.
It may be not worth changing either to simplify back-patching.

Some comments in the code still mention FALSE or TRUE:
- hashsearch.c uses FALSE in some comments.
- In execExpr.c, ExecCheck mentions TRUE.

That one is an SQL TRUE, so I left it.

Oops. You are right. I tried to be careful with what was referring to SQL and C.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers