GinPageIs* don't actually return a boolean
Hi,
#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
...
These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.
If there's actually a boolean type defined by some included header (in
which case we don't overwrite it in c.h!) this actually can lead to
tests failing. If e.g. stdbool.h is included in c.h the tests fail with
gcc-4.9.
I think we should add a !! to these macros to make sure it's an actual
boolean.
This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 .
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-11 17:42:37 +0200, Andres Freund wrote:
Hi,
#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
...These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.If there's actually a boolean type defined by some included header (in
which case we don't overwrite it in c.h!) this actually can lead to
tests failing. If e.g. stdbool.h is included in c.h the tests fail with
gcc-4.9.
I guess the reason here is that if it's a boolean type known to the
compiler it's free to assume that it only contains true/false...
I think we should add a !! to these macros to make sure it's an actual
boolean.This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 .
Even worse, we have that kind of macro all over. I thought
e.g. HeapTupleHeaderIs* would be safe agains that, but that turns out
not be the case.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund <andres@anarazel.de> wrote:
#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
...These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.If there's actually a boolean type defined by some included header (in
which case we don't overwrite it in c.h!) this actually can lead to
tests failing. If e.g. stdbool.h is included in c.h the tests fail with
gcc-4.9.
!! is unknown to our codebase except where you've added it, and
personally, I hate that idiom. I think we should write (val) != 0
instead of !!val.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-11 12:04:48 -0400, Robert Haas wrote:
On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund <andres@anarazel.de> wrote:
#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
...These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.If there's actually a boolean type defined by some included header (in
which case we don't overwrite it in c.h!) this actually can lead to
tests failing. If e.g. stdbool.h is included in c.h the tests fail with
gcc-4.9.!! is unknown to our codebase except where you've added it, and
personally, I hate that idiom. I think we should write (val) != 0
instead of !!val.
Hm. I find !! slightly simpler and it's pretty widely used in other
projects, but I don't care much. As long as we fix the underlying issue,
which != 0 certainly does...
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.
Agreed, this is risky. For example, if the bit being tested is to the
left of the lowest byte of "flags", storing the result into a bool
variable would do the wrong thing.
I think we should add a !! to these macros to make sure it's an actual
boolean.
Please write it more like
#define GinPageIsLeaf(page) ((GinPageGetOpaque(page)->flags & GIN_LEAF) != 0)
We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.
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 Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.
We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
favor of getting rid of those.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
favor of getting rid of those.
And a bunch more places actually. Blame me. I'll come up with a patch
fixing the macros and converting existing !! style ones.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund wrote:
On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
favor of getting rid of those.And a bunch more places actually. Blame me. I'll come up with a patch
fixing the macros and converting existing !! style ones.
Actually there's one that's not yours ...
alvin: master 0$ git grep '!!' -- \*.c | grep -v '!!!'
contrib/isn/isn.c: * It's a helper function, not intended to be used!!
contrib/sepgsql/uavc.c: sepgsql_audit_log(!!denied,
src/backend/access/gist/gist.c: /* yes!!, found */
src/backend/access/gist/gist.c: * awful!!, we need search tree to find parent ... , but before we
src/backend/access/gist/gistbuild.c: /* yes!!, found it */
src/backend/access/transam/xact.c: Assert(!!(parsed->xinfo & XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId));
src/backend/executor/nodeModifyTable.c: * ctid!! */
src/backend/replication/logical/reorderbuffer.c: Assert(!create || !!txn);
src/backend/storage/lmgr/lwlock.c: !!(state & LW_VAL_EXCLUSIVE),
src/backend/storage/lmgr/lwlock.c: !!(state & LW_FLAG_HAS_WAITERS),
src/backend/storage/lmgr/lwlock.c: !!(state & LW_FLAG_RELEASE_OK))));
src/backend/tsearch/wparser_def.c: * order must be the same as in typedef enum {} TParserState!!
src/backend/utils/adt/like.c: * A big hack of the regexp.c code!! Contributed by
src/bin/pg_ctl/pg_ctl.c: * manager checkpoint, it's got nothing to do with database checkpoints!!
src/interfaces/ecpg/preproc/c_keywords.c: * !!WARNING!!: This list must be sorted, because binary
src/interfaces/ecpg/preproc/ecpg_keywords.c: * !!WARNING!!: This list must be sorted, because binary
src/pl/plpgsql/src/pl_scanner.c: * !!WARNING!!: These lists must be sorted by ASCII name, because binary
--
�lvaro Herrera 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 2015-08-11 13:49:19 -0300, Alvaro Herrera wrote:
Andres Freund wrote:
On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
favor of getting rid of those.
Those got already removed by Heikki's WAL format changes...
And a bunch more places actually. Blame me. I'll come up with a patch
fixing the macros and converting existing !! style ones.Actually there's one that's not yours ...
I'm not touching sepgsql... ;)
I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.
That patch also changes !! tests into != 0 style.
Greetings,
Andres Freund
Attachments:
0001-Return-only-0-1-from-boolean-macros.patchtext/x-patch; charset=us-asciiDownload+51-51
Andres Freund <andres@anarazel.de> writes:
I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.
That patch also changes !! tests into != 0 style.
Looks OK to me, except I wonder why you did this
#define TRIGGER_FIRED_FOR_ROW(event) \
- ((event) & TRIGGER_EVENT_ROW)
+ (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)rather than != 0. That way doesn't look either more efficient or
more readable.
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 2015-08-12 18:52:59 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.That patch also changes !! tests into != 0 style.
Looks OK to me, except I wonder why you did this
#define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) & TRIGGER_EVENT_ROW) + (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)rather than != 0. That way doesn't look either more efficient or
more readable.
Purely consistency with the surrounding code. I was on the fence about
that one...
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2015-08-12 18:52:59 -0400, Tom Lane wrote:
Looks OK to me, except I wonder why you did this
#define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) & TRIGGER_EVENT_ROW) + (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)rather than != 0. That way doesn't look either more efficient or
more readable.
Purely consistency with the surrounding code. I was on the fence about
that one...
The adjacent code is doing something different than a bit-test, though:
it's checking whether multibit fields have particular values.
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 2015-08-12 19:03:50 -0400, Tom Lane wrote:
The adjacent code is doing something different than a bit-test, though:
it's checking whether multibit fields have particular values.
Yea, I know, that's why I was on the fence about it. Since you have an
opinion and I couldn't really decide it's pretty clear which direction
to go ;)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund wrote:
I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.That patch also changes !! tests into != 0 style.
I don't think you pushed this, did you?
--
�lvaro Herrera 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 вторник, 29 сентября 2015 г. 19:02:59 MSK, Alvaro Herrera wrote:
Andres Freund wrote:
I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.That patch also changes !! tests into != 0 style.
I don't think you pushed this, did you?
Hello hackers!
I've just run into a problem with these macro. Function ginStepRight breaks
completely when compiled using the MSVC2013 and MSVC2015 (since these
releases use C99's bools but without stdbool.h like C++).
I don't understand why the patch has not been commited yet. It seems to me
not so important !! or ! = 0, the solution is all that matters.
Thanks!
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
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, Feb 10, 2016 at 1:17 AM, Yury Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:
I've just run into a problem with these macro. Function ginStepRight breaks
completely when compiled using the MSVC2013 and MSVC2015 (since these
releases use C99's bools but without stdbool.h like C++).
I don't understand why the patch has not been commited yet. It seems to me
not so important !! or ! = 0, the solution is all that matters.
+1 for applying it. There were some conflicts in gist and lwlock, that
I fixed as per the attached. I have added as well an entry in next CF:
https://commitfest.postgresql.org/9/507/
If a committer wants to pick up that before, feel free. For now it
won't fall in the void, that's better than nothing.
--
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 Tue, Feb 9, 2016 at 11:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Feb 10, 2016 at 1:17 AM, Yury Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:I've just run into a problem with these macro. Function ginStepRight breaks
completely when compiled using the MSVC2013 and MSVC2015 (since these
releases use C99's bools but without stdbool.h like C++).
I don't understand why the patch has not been commited yet. It seems to me
not so important !! or ! = 0, the solution is all that matters.+1 for applying it. There were some conflicts in gist and lwlock, that
I fixed as per the attached. I have added as well an entry in next CF:
https://commitfest.postgresql.org/9/507/
If a committer wants to pick up that before, feel free. For now it
won't fall in the void, that's better than nothing.
Not actually attached.
Are we thinking to back-patch this? I would be disinclined to
back-patch widespread changes like this. If there's a specific
instance related to Gin where this is causing a tangible problem, we
could back-patch just that part, with a clear description of that
problem. Otherwise, I think this should be master-only.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-02-11 08:50:41 -0500, Robert Haas wrote:
Are we thinking to back-patch this? I would be disinclined to
back-patch widespread changes like this. If there's a specific
instance related to Gin where this is causing a tangible problem, we
could back-patch just that part, with a clear description of that
problem. Otherwise, I think this should be master-only.
I'm not sure. It's pretty darn nasty that right now we fail in some
places in the code if stdbool.h is included previously. That's probably
going to become more and more common. On the other hand it's invasive,
as you say. Partially patching things doesn't seem like a really viable
approach to me, bugs caused by this are hard to find/trigger.
- Andres
--
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, Feb 11, 2016 at 9:15 AM, Andres Freund <andres@anarazel.de> wrote:
On 2016-02-11 08:50:41 -0500, Robert Haas wrote:
Are we thinking to back-patch this? I would be disinclined to
back-patch widespread changes like this. If there's a specific
instance related to Gin where this is causing a tangible problem, we
could back-patch just that part, with a clear description of that
problem. Otherwise, I think this should be master-only.I'm not sure. It's pretty darn nasty that right now we fail in some
places in the code if stdbool.h is included previously. That's probably
going to become more and more common. On the other hand it's invasive,
as you say. Partially patching things doesn't seem like a really viable
approach to me, bugs caused by this are hard to find/trigger.
I have never been quite clear on what you think is going to cause
stdbool.h inclusions to become more common.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-02-11 09:51:26 -0500, Robert Haas wrote:
I have never been quite clear on what you think is going to cause
stdbool.h inclusions to become more common.
Primarily because it's finally starting to be supported across the
board, thanks to MS finally catching up.
Then there's uglyness like:
http://archives.postgresql.org/message-id/508bd3a0-b0a1-4f10-ad9c-2f86656f6e79%40postgrespro.ru
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
