Re: [HACKERS] taking stdbool.h into use
On Thu, Nov 9, 2017 at 1:46 AM, Peter Eisentraut 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
Re: [HACKERS] taking stdbool.h into use
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
Re: [HACKERS] taking stdbool.h into use
On 10/29/17 08:50, Michael Paquier wrote: > On Thu, Oct 26, 2017 at 9:25 AM, 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 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
Re: [HACKERS] taking stdbool.h into use
On Thu, Oct 26, 2017 at 9:25 AM, 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 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
Re: [HACKERS] taking stdbool.h into use
On Thu, Oct 26, 2017 at 5:41 PM, Tom Lane 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 https://www.postgresql.org/message-id/20160212144735.7zkg5527i3un3254%40alap3.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
Re: [HACKERS] taking stdbool.h into use
Alvaro Herrera 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: >> boolisprimary = (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
Re: [HACKERS] taking stdbool.h into use
On Thu, Oct 26, 2017 at 3:48 PM, Alvaro Herrera 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
Re: [HACKERS] taking stdbool.h into use
Michael Paquier wrote: > On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera > wrote: > > I gave this a quick run, to see if my compiler would complain for things > > like this: > > > >boolisprimary = flags & INDEX_CREATE_IS_PRIMARY; > > > > (taken from the first patch at > > https://postgr.es/m/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: > boolisprimary = (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 Herrerahttps://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
Re: [HACKERS] taking stdbool.h into use
On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera wrote: > I gave this a quick run, to see if my compiler would complain for things > like this: > >boolisprimary = flags & INDEX_CREATE_IS_PRIMARY; > > (taken from the first patch at > https://postgr.es/m/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: boolisprimary = (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
Re: [HACKERS] taking stdbool.h into use
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: boolisprimary = flags & INDEX_CREATE_IS_PRIMARY; (taken from the first patch at https://postgr.es/m/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 Herrerahttps://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
Re: [HACKERS] taking stdbool.h into use
On 9/14/17 22:35, Tom Lane wrote: > Peter Eisentraut 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
Re: [HACKERS] taking stdbool.h into use
Peter Eisentraut 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
Re: [HACKERS] taking stdbool.h into use
Peter Eisentraut 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 , 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 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
Re: [HACKERS] taking stdbool.h into use
On Wed, Aug 16, 2017 at 11:20 PM, Tom Lane 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
Re: [HACKERS] taking stdbool.h into use
I wrote: > gaur/pademelon isn't booted up right now, but it might provide > an example of a system that lacks 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 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 " 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
Re: [HACKERS] taking stdbool.h into use
Thomas Munro 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 #include 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 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
Re: [HACKERS] taking stdbool.h into use
On Wed, Aug 16, 2017 at 4:36 PM, Peter Eisentraut 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: boolhave_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/plp