strange case of "if ((a & b))"
These look strange to me - the inner parens don't do anything.
I wouldn't write it with 2x parens for the same reason I wouldn't write it with
8x parens.
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 9f159eb3db..3bbc13c443 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -693,7 +693,7 @@ check_tuple_header(HeapCheckContext *ctx)
report_corruption(ctx,
psprintf("tuple data
should begin at byte %u, but actually begins at byte %u (1 attribute, has
nulls)",
expected_hoff, ctx->tuphdr->t_hoff));
- else if ((infomask & HEAP_HASNULL))
+ else if ((infomask & HEAP_HASNULL) != 0)
report_corruption(ctx,
psprintf("tuple data
should begin at byte %u, but actually begins at byte %u (%u attributes, has
nulls)",
expected_hoff, ctx->tuphdr->t_hoff, ctx->natts));
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index 15115cb29f..0dd2838f8b 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -661,17 +661,17 @@ deparse_lquery(const lquery *in)
}
memcpy(ptr, curtlevel->name, curtlevel->len);
ptr += curtlevel->len;
- if ((curtlevel->flag & LVAR_SUBLEXEME))
+ if ((curtlevel->flag & LVAR_SUBLEXEME) != 0)
{
*ptr = '%';
ptr++;
}
- if ((curtlevel->flag & LVAR_INCASE))
+ if ((curtlevel->flag & LVAR_INCASE) != 0)
{
*ptr = '@';
ptr++;
}
- if ((curtlevel->flag & LVAR_ANYEND))
+ if ((curtlevel->flag & LVAR_ANYEND) != 0)
{
*ptr = '*';
ptr++;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 13396eb7f2..f5a4db5c57 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2107,7 +2107,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId
cid,
vmstatus = visibilitymap_get_status(relation,
BufferGetBlockNumber(buffer), &vmbuffer);
- if ((starting_with_empty_page || vmstatus &
VISIBILITYMAP_ALL_FROZEN))
+ if (starting_with_empty_page ||
+ (vmstatus & VISIBILITYMAP_ALL_FROZEN) != 0)
all_frozen_set = true;
}
diff --git a/src/backend/access/transam/xact.c
b/src/backend/access/transam/xact.c
index 441445927e..28fdd2943b 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2417,7 +2417,7 @@ PrepareTransaction(void)
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has
operated on temporary objects")));
@@ -5530,7 +5530,7 @@ XactLogCommitRecord(TimestampTz commit_time,
xl_xinfo.xinfo |= XACT_COMPLETION_UPDATE_RELCACHE_FILE;
if (forceSyncCommit)
xl_xinfo.xinfo |= XACT_COMPLETION_FORCE_SYNC_COMMIT;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK) != 0)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
/*
@@ -5681,7 +5681,7 @@ XactLogAbortRecord(TimestampTz abort_time,
xlrec.xact_time = abort_time;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK) != 0)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
if (nsubxacts > 0)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e717ada28..f341e6d143 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16029,7 +16029,7 @@ PreCommit_on_commit_actions(void)
Re: strange case of "if ((a & b))"
Justin Pryzby writes: > These look strange to me - the inner parens don't do anything. > I wouldn't write it with 2x parens for the same reason I wouldn't write it > with > 8x parens. Agreed, but shouldn't we just drop the excess parens rather than doubling down on useless notation? regards, tom lane
Re: strange case of "if ((a & b))"
On Wed, Apr 28, 2021 at 01:29:36PM -0500, Justin Pryzby wrote: > These look strange to me - the inner parens don't do anything. > I wouldn't write it with 2x parens for the same reason I wouldn't write it > with > 8x parens. > } > - else if ((!ctx->fast_forward)) > + else if ((!ctx->fast_forward) != 0) I find this part of the change harder to understand. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote: > Justin Pryzby writes: >> These look strange to me - the inner parens don't do anything. >> I wouldn't write it with 2x parens for the same reason I wouldn't write it >> with >> 8x parens. > > Agreed, but shouldn't we just drop the excess parens rather than > doubling down on useless notation? Using a notation like ((a & b) != 0) to enforce a boolean check after the bitwise operation is the usual notation I've preferred, FWIW. Do you mean something different here? -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
Michael Paquier writes: > On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote: >> Agreed, but shouldn't we just drop the excess parens rather than >> doubling down on useless notation? > Using a notation like ((a & b) != 0) to enforce a boolean check after > the bitwise operation is the usual notation I've preferred, FWIW. Do > you mean something different here? Yeah --- the "!= 0" is pointless in the context of an if-test. regards, tom lane
Re: strange case of "if ((a & b))"
On Wed, Aug 18, 2021 at 11:08:57PM -0400, Tom Lane wrote:
> Peter Smith writes:
> > On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby wrote:
> >> - state->oneCol = (origTupdesc->natts == 1) ? true : false;
> >> + state->oneCol = origTupdesc->natts == 1;
>
> FWIW, I am definitely not a fan of removing the parentheses in this
> context, because readers might wonder if you meant an "a = b = 1"
> multiple-assignment, or even misread it as that and be confused.
> So I'd prefer
>
> state->oneCol = (origTupdesc->natts == 1);
>
> In the context of "return (a == b)", I'm about neutral on whether
> to keep the parens or not, but I wonder why this patch does some
> of one and some of the other.
>
> I do agree that "x ? true : false" is silly in contexts where x
> is guaranteed to yield zero or one. What you need to be careful
> about is where x might yield other bitpatterns, for example
> "(flags & SOMEFLAG) ? true : false". Pre-C99, this type of coding
> was often *necessary*. With C99, it's only necessary if you're
> not sure that the compiler will cast the result to boolean.
I revised the patch based on these comments. I think my ternary patch already
excluded the cases that test something other than a boolean.
Peter: you quoted my patch but didn't comment on it. Your regex finds a lot of
conditional boolean assignments, but I agree that they're best left alone. My
patches are to clean up silly cases, not to rewrite things in a way that's
arguably better (but arguably not worth changing and so also not worth arguing
that it's better).
--
Justin
>From 51dab66eb3cbeccfa5bad0f1b8a26b94523edb65 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Wed, 28 Apr 2021 14:12:49 -0500
Subject: [PATCH 1/2] Avoid double parens
git grep -l '\tuphdr->t_hoff));
- else if ((infomask & HEAP_HASNULL))
+ else if (infomask & HEAP_HASNULL)
report_corruption(ctx,
psprintf("tuple data should begin at byte %u, but actually begins at byte %u (%u attributes, has nulls)",
expected_hoff, ctx->tuphdr->t_hoff, ctx->natts));
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index 15115cb29f..b75f35d5b5 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -661,17 +661,17 @@ deparse_lquery(const lquery *in)
}
memcpy(ptr, curtlevel->name, curtlevel->len);
ptr += curtlevel->len;
-if ((curtlevel->flag & LVAR_SUBLEXEME))
+if (curtlevel->flag & LVAR_SUBLEXEME)
{
*ptr = '%';
ptr++;
}
-if ((curtlevel->flag & LVAR_INCASE))
+if (curtlevel->flag & LVAR_INCASE)
{
*ptr = '@';
ptr++;
}
-if ((curtlevel->flag & LVAR_ANYEND))
+if (curtlevel->flag & LVAR_ANYEND)
{
*ptr = '*';
ptr++;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 387f80419a..1ba9dbf966 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2417,7 +2417,7 @@ PrepareTransaction(void)
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+ if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
@@ -5530,7 +5530,7 @@ XactLogCommitRecord(TimestampTz commit_time,
xl_xinfo.xinfo |= XACT_COMPLETION_UPDATE_RELCACHE_FILE;
if (forceSyncCommit)
xl_xinfo.xinfo |= XACT_COMPLETION_FORCE_SYNC_COMMIT;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
/*
@@ -5681,7 +5681,7 @@ XactLogAbortRecord(TimestampTz abort_time,
xlrec.xact_time = abort_time;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
if (nsubxacts > 0)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dbee6ae199..e018cdfd9e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16309,7 +16309,7 @@ PreCommit_on_commit_actions(void)
* relations, we can skip truncating ON COMMIT DELETE ROWS
* tables, as they must still be empty.
*/
-if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
break;
case ONCOMMIT_DROP:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index ff3dcc7b18..ae3364dfdc 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2390,7 +2390,7 @@ query_tree_walker(Query *query,
* don't contain actual expressions. However they do contain OIDs which
* may be ne
Re: strange case of "if ((a & b))"
On Sun, Sep 05, 2021 at 07:11:10PM -0500, Justin Pryzby wrote: > I revised the patch based on these comments. I think my ternary patch already > excluded the cases that test something other than a boolean. In 0002, everything is a boolean expression except for SpGistPageStoresNulls() and GistPageIsLeaf(). So that's a good cleanup overall. - pathnode->parallel_aware = parallel_workers > 0 ? true : false; + pathnode->parallel_aware = parallel_workers > 0; I also prefer that we keep the parenthesis for such things. That's more readable and easier to reason about. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Tue, Sep 07, 2021 at 02:59:58PM +0900, Michael Paquier wrote: > In 0002, everything is a boolean expression except for > SpGistPageStoresNulls() and GistPageIsLeaf(). So that's a good > cleanup overall. I looked again at 0002 again yesterday, and that was an improvement for most of those locations, where we already use a boolean as expression, so done mostly as of fd0625c. > - pathnode->parallel_aware = parallel_workers > 0 ? true : false; > + pathnode->parallel_aware = parallel_workers > 0; > I also prefer that we keep the parenthesis for such things. That's > more readable and easier to reason about. Adjusted these as well. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
At Thu, 9 Sep 2021 13:28:54 +0900, Michael Paquier wrote in > On Tue, Sep 07, 2021 at 02:59:58PM +0900, Michael Paquier wrote: > > In 0002, everything is a boolean expression except for > > SpGistPageStoresNulls() and GistPageIsLeaf(). So that's a good > > cleanup overall. > > I looked again at 0002 again yesterday, and that was an improvement > for most of those locations, where we already use a boolean as > expression, so done mostly as of fd0625c. > > > - pathnode->parallel_aware = parallel_workers > 0 ? true : false; > > + pathnode->parallel_aware = parallel_workers > 0; > > I also prefer that we keep the parenthesis for such things. That's > > more readable and easier to reason about. > > Adjusted these as well. Maybe I'm missing something, but I can see several instances of the "eval-bool ? true : false" pattern after fd0625c7a9 that are not in the latest 0002. ./backend/nodes/readfuncs.c
Re: strange case of "if ((a & b))"
On Thu, Sep 09, 2021 at 02:14:50PM +0900, Kyotaro Horiguchi wrote: > Maybe I'm missing something, but I can see several instances of the > "eval-bool ? true : false" pattern after fd0625c7a9 that are not in > the latest 0002. Yep. There are more of these, and I have just looked at some of them as of the patches proposed. What was sent looked clean enough to progress a bit and be done with it. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Sat, Sep 11, 2021 at 2:44 PM Michael Paquier wrote: > > On Thu, Sep 09, 2021 at 02:14:50PM +0900, Kyotaro Horiguchi wrote: > > Maybe I'm missing something, but I can see several instances of the > > "eval-bool ? true : false" pattern after fd0625c7a9 that are not in > > the latest 0002. > > Yep. There are more of these, and I have just looked at some of them > as of the patches proposed. What was sent looked clean enough to > progress a bit and be done with it. While reading the decode.c I found the extra parentheses and arrived at this thread. The discussion seems to get inactive now but one (0001 patch) out of two patches Justin proposed [1] is not committed yet and there seems no CF entry for this item (0002 patch already got committed, fd0625c7a9). 0001 patch can be cleanly applied and looks good to me. Also, regarding "x ? true: false" pattern where x is guaranteed to yield a boolean, I found other examples other than Horiguchi-san mentioned[2]. I've attached the patch to remove them. Regards, [1] https://www.postgresql.org/message-id/20210906001110.GF26465%40telsasoft.com [2] https://www.postgresql.org/message-id/20210909.141450.11969674682374713.horikyota.ntt%40gmail.com -- Masahiko Sawada EDB: https://www.enterprisedb.com/ remove_unnecessary_ternary_operations.patch Description: Binary data
Re: strange case of "if ((a & b))"
On Thu, Oct 07, 2021 at 11:18:24AM +0900, Masahiko Sawada wrote: > On Sat, Sep 11, 2021 at 2:44 PM Michael Paquier wrote: > > > > On Thu, Sep 09, 2021 at 02:14:50PM +0900, Kyotaro Horiguchi wrote: > > > Maybe I'm missing something, but I can see several instances of the > > > "eval-bool ? true : false" pattern after fd0625c7a9 that are not in > > > the latest 0002. > > > > Yep. There are more of these, and I have just looked at some of them > > as of the patches proposed. What was sent looked clean enough to > > progress a bit and be done with it. > > While reading the decode.c I found the extra parentheses and arrived > at this thread. I'm not quite sure how you managed to search for it - well done ;) > The discussion seems to get inactive now but one (0001 > patch) out of two patches Justin proposed [1] is not committed yet and > there seems no CF entry for this item (0002 patch already got > committed, fd0625c7a9). 0001 patch can be cleanly applied and looks > good to me. Note that I also included it here: https://www.postgresql.org/message-id/20210924215827.gs...@telsasoft.com Michael seems prefer writing (() != 0) in more cases than other people, so didn't care for that patch. https://www.postgresql.org/message-id/577206.1620628...@sss.pgh.pa.us -- Justin
Re: strange case of "if ((a & b))"
On Thu, Oct 7, 2021 at 11:44 AM Justin Pryzby wrote: > > On Thu, Oct 07, 2021 at 11:18:24AM +0900, Masahiko Sawada wrote: > > On Sat, Sep 11, 2021 at 2:44 PM Michael Paquier wrote: > > > > > > On Thu, Sep 09, 2021 at 02:14:50PM +0900, Kyotaro Horiguchi wrote: > > > > Maybe I'm missing something, but I can see several instances of the > > > > "eval-bool ? true : false" pattern after fd0625c7a9 that are not in > > > > the latest 0002. > > > > > > Yep. There are more of these, and I have just looked at some of them > > > as of the patches proposed. What was sent looked clean enough to > > > progress a bit and be done with it. > > > > While reading the decode.c I found the extra parentheses and arrived > > at this thread. > > I'm not quite sure how you managed to search for it - well done ;) I could not find the recent thread, though :) > > > The discussion seems to get inactive now but one (0001 > > patch) out of two patches Justin proposed [1] is not committed yet and > > there seems no CF entry for this item (0002 patch already got > > committed, fd0625c7a9). 0001 patch can be cleanly applied and looks > > good to me. > > Note that I also included it here: > https://www.postgresql.org/message-id/20210924215827.gs...@telsasoft.com Good. Thank you for the information! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: strange case of "if ((a & b))"
On Thu, Oct 07, 2021 at 01:27:34PM +0900, Masahiko Sawada wrote: > On Thu, Oct 7, 2021 at 11:44 AM Justin Pryzby wrote: >> I'm not quite sure how you managed to search for it - well done ;) > > I could not find the recent thread, though :) Hm. It looks like there are more occurences of "false : true" that could be cleaned up, like in nodeResult.c or tablecmds.c. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Thu, Oct 7, 2021 at 1:37 PM Michael Paquier wrote: > > On Thu, Oct 07, 2021 at 01:27:34PM +0900, Masahiko Sawada wrote: > > On Thu, Oct 7, 2021 at 11:44 AM Justin Pryzby wrote: > >> I'm not quite sure how you managed to search for it - well done ;) > > > > I could not find the recent thread, though :) > > Hm. It looks like there are more occurences of "false : true" that > could be cleaned up, like in nodeResult.c or tablecmds.c. Indeed. I've attached a patch that also deals with "false : true" cases. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ remove_unnecessary_ternary_operations_v2.patch Description: Binary data
Re: strange case of "if ((a & b))"
On Thu, Oct 07, 2021 at 03:24:53PM +0900, Masahiko Sawada wrote: > Indeed. I've attached a patch that also deals with "false : true" cases. Looks right. I would be tempted to keep the one in readfuncs.c though, mostly as a matter of style, and I would add a comparison with NULL for the return result of bsearch() in ts_utils.c. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Thu, Oct 07, 2021 at 04:49:10PM +0900, Michael Paquier wrote: > Looks right. I would be tempted to keep the one in readfuncs.c > though, mostly as a matter of style. I have left this one alone, and applied the rest as of 68f7c4b. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Mon, Oct 11, 2021 at 9:45 AM Michael Paquier wrote: > > On Thu, Oct 07, 2021 at 04:49:10PM +0900, Michael Paquier wrote: > > Looks right. I would be tempted to keep the one in readfuncs.c > > though, mostly as a matter of style. > > I have left this one alone, and applied the rest as of 68f7c4b. Thank you! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: strange case of "if ((a & b))"
On Thu, Jun 24, 2021 at 09:31:11PM -0500, Justin Pryzby wrote: > On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote: > > Justin Pryzby writes: > > > These look strange to me - the inner parens don't do anything. > > > I wouldn't write it with 2x parens for the same reason I wouldn't write > > > it with > > > 8x parens. > > > > Agreed, but shouldn't we just drop the excess parens rather than > > doubling down on useless notation? > > I believe I got the impression from Michael that there was a style preference > to write != 0. > > 0002 is a bonus patch I found in my typos branch. I will hold onto it for > later if nobody wants to deal with it. I am ready to deal with this patch. Should I apply it to master soon? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: strange case of "if ((a & b))"
On Wed, Aug 18, 2021 at 02:02:22PM -0400, Bruce Momjian wrote:
> On Thu, Jun 24, 2021 at 09:31:11PM -0500, Justin Pryzby wrote:
> > On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote:
> > > Justin Pryzby writes:
> > > > These look strange to me - the inner parens don't do anything.
> > > > I wouldn't write it with 2x parens for the same reason I wouldn't write
> > > > it with
> > > > 8x parens.
> > >
> > > Agreed, but shouldn't we just drop the excess parens rather than
> > > doubling down on useless notation?
> >
> > I believe I got the impression from Michael that there was a style
> > preference
> > to write != 0.
> >
> > 0002 is a bonus patch I found in my typos branch. I will hold onto it for
> > later if nobody wants to deal with it.
>
> I am ready to deal with this patch. Should I apply it to master soon?
Thanks for looking at it. I suggest not to apply 0002 - I'll resend it on
another thread with other, similar cleanups.
However, I have another patch to clean up stuff like "? true : false", which
seems related to this patch (but maybe it should be applied separately).
commit 85952c0e1621a5a491a9422cdee66e733728e3a8
Author: Justin Pryzby
Date: Fri May 7 08:16:51 2021 -0500
Avoid verbose ternary operator with expressions which are already boolean
diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
index 91690aff51..8ed4d63fc3 100644
--- a/contrib/intarray/_int_tool.c
+++ b/contrib/intarray/_int_tool.c
@@ -41,7 +41,7 @@ inner_int_contains(ArrayType *a, ArrayType *b)
break; /* db[j] is not in da */
}
- return (n == nb) ? true : false;
+ return (n == nb);
}
/* arguments are assumed sorted */
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 6cf181bc53..7c39ed4298 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -137,7 +137,7 @@ ltree_same(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(result);
if (LTG_ISONENODE(a))
- *result = (ISEQ(LTG_NODE(a), LTG_NODE(b))) ? true : false;
+ *result = ISEQ(LTG_NODE(a), LTG_NODE(b));
else
{
int32 i;
diff --git a/contrib/sepgsql/selinux.c b/contrib/sepgsql/selinux.c
index f11968bcaa..dac3f3ec91 100644
--- a/contrib/sepgsql/selinux.c
+++ b/contrib/sepgsql/selinux.c
@@ -615,7 +615,7 @@ static int sepgsql_mode = SEPGSQL_MODE_INTERNAL;
bool
sepgsql_is_enabled(void)
{
- return (sepgsql_mode != SEPGSQL_MODE_DISABLED ? true : false);
+ return sepgsql_mode != SEPGSQL_MODE_DISABLED;
}
/*
diff --git a/src/backend/access/gin/gindatapage.c
b/src/backend/access/gin/gindatapage.c
index 06c0586543..2ada1dcbda 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -241,7 +241,7 @@ dataIsMoveRight(GinBtree btree, Page page)
if (GinPageIsDeleted(page))
return true;
- return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true :
false;
+ return ginCompareItemPointers(&btree->itemptr, iptr) > 0;
}
/*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index cdd626ff0a..5b054ef4ae 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -100,7 +100,7 @@ initGinState(GinState *state, Relation index)
MemSet(state, 0, sizeof(GinState));
state->index = index;
- state->oneCol = (origTupdesc->natts == 1) ? true : false;
+ state->oneCol = origTupdesc->natts == 1;
state->origTupdesc = origTupdesc;
for (i = 0; i < origTupdesc->natts; i++)
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0683f42c25..a83a2e9952 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -231,7 +231,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE
*giststate,
{
BlockNumber blkno = BufferGetBlockNumber(buffer);
Pagepage = BufferGetPage(buffer);
- boolis_leaf = (GistPageIsLeaf(page)) ? true : false;
+ boolis_leaf = GistPageIsLeaf(page);
XLogRecPtr recptr;
int i;
boolis_split;
diff --git a/src/backend/access/gist/gistsplit.c
b/src/backend/access/gist/gistsplit.c
index 526ed1218e..853ebc387b 100644
--- a/src/backend/access/gist/gistsplit.c
+++ b/src/backend/access/gist/gistsplit.c
@@ -303,9 +303,9 @@ supportSecondarySplit(Relation r, GISTSTATE *giststate, int
attno,
penalty2 = gistpenalty(giststate, attno, entry1, false,
&entrySR, false);
if (penalty1 < penalty2)
- leaveOnLeft = (sv->spl_ldatum_exists) ? true : false;
+ leaveOnLeft = sv->spl_ldatum_exists;
else
- leaveOnLeft = (sv->spl_rdatum_exists) ? true : false;
+ le
Re: strange case of "if ((a & b))"
On Wed, Aug 18, 2021 at 01:28:56PM -0500, Justin Pryzby wrote: > > > 0002 is a bonus patch I found in my typos branch. I will hold onto it for > > > later if nobody wants to deal with it. > > > > I am ready to deal with this patch. Should I apply it to master soon? > > Thanks for looking at it. I suggest not to apply 0002 - I'll resend it on > another thread with other, similar cleanups. OK. > However, I have another patch to clean up stuff like "? true : false", which > seems related to this patch (but maybe it should be applied separately). Yes, that is odd. I think it is related to the confusion that if () compares non-zero(true) and zero(false), while booleans return only 1/0 (no other values). This explores that: https://stackoverflow.com/questions/22489517/c-language-boolean-expression-return-value Do you want me to consider this patch now? -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: strange case of "if ((a & b))"
On Wed, Aug 18, 2021 at 03:15:21PM -0400, Bruce Momjian wrote: > On Wed, Aug 18, 2021 at 01:28:56PM -0500, Justin Pryzby wrote: > > > > 0002 is a bonus patch I found in my typos branch. I will hold onto it > > > > for > > > > later if nobody wants to deal with it. > > > > > > I am ready to deal with this patch. Should I apply it to master soon? > > > > Thanks for looking at it. I suggest not to apply 0002 - I'll resend it on > > another thread with other, similar cleanups. > > OK. > > > However, I have another patch to clean up stuff like "? true : false", which > > seems related to this patch (but maybe it should be applied separately). > > Yes, that is odd. I think it is related to the confusion that if () > compares non-zero(true) and zero(false), while booleans return only 1/0 > (no other values). This explores that: > > > https://stackoverflow.com/questions/22489517/c-language-boolean-expression-return-value > > Do you want me to consider this patch now? Yes, please. It may be helpful to dispose of the first patch first. Thanks, Justin
Re: strange case of "if ((a & b))"
On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby wrote:
>
> On Wed, Aug 18, 2021 at 02:02:22PM -0400, Bruce Momjian wrote:
> > On Thu, Jun 24, 2021 at 09:31:11PM -0500, Justin Pryzby wrote:
> > > On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote:
> > > > Justin Pryzby writes:
> > > > > These look strange to me - the inner parens don't do anything.
> > > > > I wouldn't write it with 2x parens for the same reason I wouldn't
> > > > > write it with
> > > > > 8x parens.
> > > >
> > > > Agreed, but shouldn't we just drop the excess parens rather than
> > > > doubling down on useless notation?
> > >
> > > I believe I got the impression from Michael that there was a style
> > > preference
> > > to write != 0.
> > >
> > > 0002 is a bonus patch I found in my typos branch. I will hold onto it for
> > > later if nobody wants to deal with it.
> >
> > I am ready to deal with this patch. Should I apply it to master soon?
>
> Thanks for looking at it. I suggest not to apply 0002 - I'll resend it on
> another thread with other, similar cleanups.
>
> However, I have another patch to clean up stuff like "? true : false", which
> seems related to this patch (but maybe it should be applied separately).
>
> commit 85952c0e1621a5a491a9422cdee66e733728e3a8
> Author: Justin Pryzby
> Date: Fri May 7 08:16:51 2021 -0500
>
> Avoid verbose ternary operator with expressions which are already boolean
>
> diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
> index 91690aff51..8ed4d63fc3 100644
> --- a/contrib/intarray/_int_tool.c
> +++ b/contrib/intarray/_int_tool.c
> @@ -41,7 +41,7 @@ inner_int_contains(ArrayType *a, ArrayType *b)
> break; /* db[j] is not in da
> */
> }
>
> - return (n == nb) ? true : false;
> + return (n == nb);
> }
>
> /* arguments are assumed sorted */
> diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
> index 6cf181bc53..7c39ed4298 100644
> --- a/contrib/ltree/ltree_gist.c
> +++ b/contrib/ltree/ltree_gist.c
> @@ -137,7 +137,7 @@ ltree_same(PG_FUNCTION_ARGS)
> PG_RETURN_POINTER(result);
>
> if (LTG_ISONENODE(a))
> - *result = (ISEQ(LTG_NODE(a), LTG_NODE(b))) ? true : false;
> + *result = ISEQ(LTG_NODE(a), LTG_NODE(b));
> else
> {
> int32 i;
> diff --git a/contrib/sepgsql/selinux.c b/contrib/sepgsql/selinux.c
> index f11968bcaa..dac3f3ec91 100644
> --- a/contrib/sepgsql/selinux.c
> +++ b/contrib/sepgsql/selinux.c
> @@ -615,7 +615,7 @@ static int sepgsql_mode = SEPGSQL_MODE_INTERNAL;
> bool
> sepgsql_is_enabled(void)
> {
> - return (sepgsql_mode != SEPGSQL_MODE_DISABLED ? true : false);
> + return sepgsql_mode != SEPGSQL_MODE_DISABLED;
> }
>
> /*
> diff --git a/src/backend/access/gin/gindatapage.c
> b/src/backend/access/gin/gindatapage.c
> index 06c0586543..2ada1dcbda 100644
> --- a/src/backend/access/gin/gindatapage.c
> +++ b/src/backend/access/gin/gindatapage.c
> @@ -241,7 +241,7 @@ dataIsMoveRight(GinBtree btree, Page page)
> if (GinPageIsDeleted(page))
> return true;
>
> - return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true :
> false;
> + return ginCompareItemPointers(&btree->itemptr, iptr) > 0;
> }
>
> /*
> diff --git a/src/backend/access/gin/ginutil.c
> b/src/backend/access/gin/ginutil.c
> index cdd626ff0a..5b054ef4ae 100644
> --- a/src/backend/access/gin/ginutil.c
> +++ b/src/backend/access/gin/ginutil.c
> @@ -100,7 +100,7 @@ initGinState(GinState *state, Relation index)
> MemSet(state, 0, sizeof(GinState));
>
> state->index = index;
> - state->oneCol = (origTupdesc->natts == 1) ? true : false;
> + state->oneCol = origTupdesc->natts == 1;
> state->origTupdesc = origTupdesc;
>
> for (i = 0; i < origTupdesc->natts; i++)
> diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
> index 0683f42c25..a83a2e9952 100644
> --- a/src/backend/access/gist/gist.c
> +++ b/src/backend/access/gist/gist.c
> @@ -231,7 +231,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE
> *giststate,
> {
> BlockNumber blkno = BufferGetBlockNumber(buffer);
> Pagepage = BufferGetPage(buffer);
> - boolis_leaf = (GistPageIsLeaf(page)) ? true : false;
> + boolis_leaf = GistPageIsLeaf(page);
> XLogRecPtr recptr;
> int i;
> boolis_split;
> diff --git a/src/backend/access/gist/gistsplit.c
> b/src/backend/access/gist/gistsplit.c
> index 526ed1218e..853ebc387b 100644
> --- a/src/backend/access/gist/gistsplit.c
> +++ b/src/backend/access/gist/gistsplit.c
> @@ -303,9 +303,9 @@ supportSecondarySplit(Relation r, GISTSTATE *giststate,
> int attno,
> penalty2 = gistpenalty(giststate, attno, entry1, false,
> &entrySR, false);
>
> if (
Re: strange case of "if ((a & b))"
Peter Smith writes: > On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby wrote: >> - state->oneCol = (origTupdesc->natts == 1) ? true : false; >> + state->oneCol = origTupdesc->natts == 1; FWIW, I am definitely not a fan of removing the parentheses in this context, because readers might wonder if you meant an "a = b = 1" multiple-assignment, or even misread it as that and be confused. So I'd prefer state->oneCol = (origTupdesc->natts == 1); In the context of "return (a == b)", I'm about neutral on whether to keep the parens or not, but I wonder why this patch does some of one and some of the other. I do agree that "x ? true : false" is silly in contexts where x is guaranteed to yield zero or one. What you need to be careful about is where x might yield other bitpatterns, for example "(flags & SOMEFLAG) ? true : false". Pre-C99, this type of coding was often *necessary*. With C99, it's only necessary if you're not sure that the compiler will cast the result to boolean. regards, tom lane
Re: strange case of "if ((a & b))"
> On 19 Aug 2021, at 05:08, Tom Lane wrote: > > Peter Smith writes: >> On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby wrote: >>> - state->oneCol = (origTupdesc->natts == 1) ? true : false; >>> + state->oneCol = origTupdesc->natts == 1; > > FWIW, I am definitely not a fan of removing the parentheses in this > context, because readers might wonder if you meant an "a = b = 1" > multiple-assignment, or even misread it as that and be confused. > So I'd prefer > > state->oneCol = (origTupdesc->natts == 1); +1, the parenthesis makes it a lot more readable IMO. -- Daniel Gustafsson https://vmware.com/
Re: strange case of "if ((a & b))"
On Wed, Aug 18, 2021 at 11:08:57PM -0400, Tom Lane wrote: > Peter Smith writes: > > On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby wrote: > >> - state->oneCol = (origTupdesc->natts == 1) ? true : false; > >> + state->oneCol = origTupdesc->natts == 1; > > FWIW, I am definitely not a fan of removing the parentheses in this > context, because readers might wonder if you meant an "a = b = 1" > multiple-assignment, or even misread it as that and be confused. > So I'd prefer > > state->oneCol = (origTupdesc->natts == 1); Good point --- extra parentheses are not always bad. > > In the context of "return (a == b)", I'm about neutral on whether > to keep the parens or not, but I wonder why this patch does some > of one and some of the other. > > I do agree that "x ? true : false" is silly in contexts where x > is guaranteed to yield zero or one. What you need to be careful > about is where x might yield other bitpatterns, for example > "(flags & SOMEFLAG) ? true : false". Pre-C99, this type of coding > was often *necessary*. With C99, it's only necessary if you're > not sure that the compiler will cast the result to boolean. Agreed. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: strange case of "if ((a & b))"
On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote:
> Justin Pryzby writes:
> > These look strange to me - the inner parens don't do anything.
> > I wouldn't write it with 2x parens for the same reason I wouldn't write it
> > with
> > 8x parens.
>
> Agreed, but shouldn't we just drop the excess parens rather than
> doubling down on useless notation?
I believe I got the impression from Michael that there was a style preference
to write != 0.
0002 is a bonus patch I found in my typos branch. I will hold onto it for
later if nobody wants to deal with it.
>From 2534ff46830ab505a6a2079a1b6c0f8a1e9b2b8b Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Wed, 28 Apr 2021 14:12:49 -0500
Subject: [PATCH 1/2] Avoid double parens
git grep -l '\tuphdr->t_hoff));
- else if ((infomask & HEAP_HASNULL))
+ else if (infomask & HEAP_HASNULL)
report_corruption(ctx,
psprintf("tuple data should begin at byte %u, but actually begins at byte %u (%u attributes, has nulls)",
expected_hoff, ctx->tuphdr->t_hoff, ctx->natts));
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index 15115cb29f..b75f35d5b5 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -661,17 +661,17 @@ deparse_lquery(const lquery *in)
}
memcpy(ptr, curtlevel->name, curtlevel->len);
ptr += curtlevel->len;
-if ((curtlevel->flag & LVAR_SUBLEXEME))
+if (curtlevel->flag & LVAR_SUBLEXEME)
{
*ptr = '%';
ptr++;
}
-if ((curtlevel->flag & LVAR_INCASE))
+if (curtlevel->flag & LVAR_INCASE)
{
*ptr = '@';
ptr++;
}
-if ((curtlevel->flag & LVAR_ANYEND))
+if (curtlevel->flag & LVAR_ANYEND)
{
*ptr = '*';
ptr++;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index da8a460722..9c13a7c589 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2452,7 +2452,7 @@ PrepareTransaction(void)
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+ if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
@@ -5565,7 +5565,7 @@ XactLogCommitRecord(TimestampTz commit_time,
xl_xinfo.xinfo |= XACT_COMPLETION_UPDATE_RELCACHE_FILE;
if (forceSyncCommit)
xl_xinfo.xinfo |= XACT_COMPLETION_FORCE_SYNC_COMMIT;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
/*
@@ -5716,7 +5716,7 @@ XactLogAbortRecord(TimestampTz abort_time,
xlrec.xact_time = abort_time;
- if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+ if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
if (nsubxacts > 0)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4e23c7fce5..39180999fd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16171,7 +16171,7 @@ PreCommit_on_commit_actions(void)
* relations, we can skip truncating ON COMMIT DELETE ROWS
* tables, as they must still be empty.
*/
-if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
break;
case ONCOMMIT_DROP:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index ff3dcc7b18..ae3364dfdc 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2390,7 +2390,7 @@ query_tree_walker(Query *query,
* don't contain actual expressions. However they do contain OIDs which
* may be needed by dependency walkers etc.
*/
- if ((flags & QTW_EXAMINE_SORTGROUP))
+ if (flags & QTW_EXAMINE_SORTGROUP)
{
if (walker((Node *) query->groupClause, context))
return true;
@@ -3328,7 +3328,7 @@ query_tree_mutator(Query *query,
* may be of interest to some mutators.
*/
- if ((flags & QTW_EXAMINE_SORTGROUP))
+ if (flags & QTW_EXAMINE_SORTGROUP)
{
MUTATE(query->groupClause, query->groupClause, List *);
MUTATE(query->windowClause, query->windowClause, List *);
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 453efc51e1..34a36b22f8 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -337,7 +337,7 @@ DecodeXactOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
buf->origptr);
}
-else if ((!ctx->fast_forward))
+else if (!ctx->fast_forward)
ReorderBufferImmediateInvalidation(ctx->reorder,
