Thread: 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)
* 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) != 0)
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..fe825c6ede 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) != 0)
{
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) != 0)
{
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 7924581cdc..62baf48f8e 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) != 0)
ReorderBufferImmediateInvalidation(ctx->reorder,
invals->nmsgs,
invals->msgs);
Justin Pryzby <pryzby@telsasoft.com> 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
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
Attachment
Michael Paquier <michael@paquier.xyz> 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
On Wed, Apr 28, 2021 at 02:40:09PM -0400, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> 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.
Attachment
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 <pryzby@telsasoft.com> 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 <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
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 <pryzby@telsasoft.com> 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 <pryzbyj@telsasoft.com>
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);
Page page = BufferGetPage(buffer);
- bool is_leaf = (GistPageIsLeaf(page)) ? true : false;
+ bool is_leaf = GistPageIsLeaf(page);
XLogRecPtr recptr;
int i;
bool is_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;
+ leaveOnLeft = sv->spl_rdatum_exists;
}
if (leaveOnLeft == false)
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 0752fb38a9..5e3730201c 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -816,7 +816,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf,
XLogRecPtr recptr;
xlrec.clear_dead_marking = clear_dead_marking;
- xlrec.is_primary_bucket_page = (buf == bucket_buf) ? true : false;
+ xlrec.is_primary_bucket_page = buf == bucket_buf;
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, SizeOfHashDelete);
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index d254a00b6a..83af8c1f67 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -176,7 +176,7 @@ restart_insert:
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
/* chain to a new overflow page */
- buf = _hash_addovflpage(rel, metabuf, buf, (buf == bucket_buf) ? true : false);
+ buf = _hash_addovflpage(rel, metabuf, buf, buf == bucket_buf);
page = BufferGetPage(buf);
/* should fit now, given test above */
diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
index 404f2b6221..7397b6963f 100644
--- a/src/backend/access/hash/hashovfl.c
+++ b/src/backend/access/hash/hashovfl.c
@@ -953,7 +953,7 @@ readpage:
xl_hash_move_page_contents xlrec;
xlrec.ntups = nitups;
- xlrec.is_prim_bucket_same_wrt = (wbuf == bucket_buf) ? true : false;
+ xlrec.is_prim_bucket_same_wrt = wbuf == bucket_buf;
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, SizeOfHashMovePageContents);
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index b730025356..eda10386b2 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -1195,7 +1195,7 @@ _hash_splitbucket(Relation rel,
all_tups_size = 0;
/* chain to a new overflow page */
- nbuf = _hash_addovflpage(rel, metabuf, nbuf, (nbuf == bucket_nbuf) ? true : false);
+ nbuf = _hash_addovflpage(rel, metabuf, nbuf, nbuf == bucket_nbuf);
npage = BufferGetPage(nbuf);
nopaque = (HashPageOpaque) PageGetSpecialPointer(npage);
}
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index d3c57cd16a..b72b03ea25 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1475,7 +1475,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
* all relevant hint bits were just set moments ago).
*/
if (!HeapTupleHeaderXminCommitted(tuple))
- return HeapTupleHeaderXminInvalid(tuple) ? true : false;
+ return HeapTupleHeaderXminInvalid(tuple);
/*
* If the inserting transaction committed, but any deleting transaction
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index e14b9fa573..037d56132e 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -860,7 +860,7 @@ redirect:
page = BufferGetPage(buffer);
TestForOldSnapshot(snapshot, index, page);
- isnull = SpGistPageStoresNulls(page) ? true : false;
+ isnull = SpGistPageStoresNulls(page);
if (SpGistPageIsLeaf(page))
{
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index bf619d3a65..23fe1d85fd 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1037,7 +1037,7 @@ SPI_modifytuple(Relation rel, HeapTuple tuple, int natts, int *attnum,
if (attnum[i] <= 0 || attnum[i] > numberOfAttributes)
break;
v[attnum[i] - 1] = Values[i];
- n[attnum[i] - 1] = (Nulls && Nulls[i] == 'n') ? true : false;
+ n[attnum[i] - 1] = Nulls && Nulls[i] == 'n';
}
if (i == natts) /* no errors in *attnum */
diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
index 2da300e000..91b8ae6c51 100644
--- a/src/backend/jit/jit.c
+++ b/src/backend/jit/jit.c
@@ -198,7 +198,7 @@ file_exists(const char *name)
AssertArg(name != NULL);
if (stat(name, &st) == 0)
- return S_ISDIR(st.st_mode) ? false : true;
+ return !S_ISDIR(st.st_mode);
else if (!(errno == ENOENT || errno == ENOTDIR))
ereport(ERROR,
(errcode_for_file_access(),
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 41cbf328c4..3f14b5c1c4 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -936,7 +936,7 @@ create_seqscan_path(PlannerInfo *root, RelOptInfo *rel,
pathnode->pathtarget = rel->reltarget;
pathnode->param_info = get_baserel_parampathinfo(root, rel,
required_outer);
- pathnode->parallel_aware = parallel_workers > 0 ? true : false;
+ pathnode->parallel_aware = parallel_workers > 0;
pathnode->parallel_safe = rel->consider_parallel;
pathnode->parallel_workers = parallel_workers;
pathnode->pathkeys = NIL; /* seqscan has unordered result */
@@ -1057,7 +1057,7 @@ create_bitmap_heap_path(PlannerInfo *root,
pathnode->path.pathtarget = rel->reltarget;
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
required_outer);
- pathnode->path.parallel_aware = parallel_degree > 0 ? true : false;
+ pathnode->path.parallel_aware = parallel_degree > 0;
pathnode->path.parallel_safe = rel->consider_parallel;
pathnode->path.parallel_workers = parallel_degree;
pathnode->path.pathkeys = NIL; /* always unordered */
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index ef118952c7..35b39ece07 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1772,7 +1772,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
for (i = 0; i < mcvlist->nitems; i++)
{
int j;
- bool match = (expr->useOr ? false : true);
+ bool match = !expr->useOr;
MCVItem *item = &mcvlist->items[i];
/*
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index a4be5fe513..41a6d4793c 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -325,7 +325,7 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode)
file = makeBufFileCommon(nfiles);
file->files = files;
- file->readOnly = (mode == O_RDONLY) ? true : false;
+ file->readOnly = mode == O_RDONLY;
file->fileset = fileset;
file->name = pstrdup(name);
diff --git a/src/backend/tsearch/ts_parse.c b/src/backend/tsearch/ts_parse.c
index d978c8850d..3ae0044dfb 100644
--- a/src/backend/tsearch/ts_parse.c
+++ b/src/backend/tsearch/ts_parse.c
@@ -288,7 +288,7 @@ LexizeExec(LexizeData *ld, ParsedLex **correspondLexem)
}
}
- ld->dictState.isend = (curVal->type == 0) ? true : false;
+ ld->dictState.isend = (curVal->type == 0);
ld->dictState.getnext = false;
res = (TSLexeme *) DatumGetPointer(FunctionCall4(&(dict->lexize),
diff --git a/src/backend/utils/adt/bool.c b/src/backend/utils/adt/bool.c
index fe11d1ae94..cd98f84270 100644
--- a/src/backend/utils/adt/bool.c
+++ b/src/backend/utils/adt/bool.c
@@ -184,7 +184,7 @@ boolrecv(PG_FUNCTION_ARGS)
int ext;
ext = pq_getmsgbyte(buf);
- PG_RETURN_BOOL((ext != 0) ? true : false);
+ PG_RETURN_BOOL(ext != 0);
}
/*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 4df8cc5abf..0f5f1208c9 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7997,14 +7997,14 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
* appears simple since . has top precedence, unless parent is
* T_FieldSelect itself!
*/
- return (IsA(parentNode, FieldSelect) ? false : true);
+ return !IsA(parentNode, FieldSelect);
case T_FieldStore:
/*
* treat like FieldSelect (probably doesn't matter)
*/
- return (IsA(parentNode, FieldStore) ? false : true);
+ return !IsA(parentNode, FieldStore);
case T_CoerceToDomain:
/* maybe simple, check args */
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index 14d7343afa..906a686914 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -109,7 +109,7 @@ gtsquery_same(PG_FUNCTION_ARGS)
TSQuerySign b = PG_GETARG_TSQUERYSIGN(1);
bool *result = (bool *) PG_GETARG_POINTER(2);
- *result = (a == b) ? true : false;
+ *result = (a == b);
PG_RETURN_POINTER(result);
}
diff --git a/src/backend/utils/adt/tsquery_util.c b/src/backend/utils/adt/tsquery_util.c
index 7f936427b5..3dcc753e98 100644
--- a/src/backend/utils/adt/tsquery_util.c
+++ b/src/backend/utils/adt/tsquery_util.c
@@ -186,7 +186,7 @@ QTNEq(QTNode *a, QTNode *b)
if (!(sign == a->sign && sign == b->sign))
return false;
- return (QTNodeCompare(a, b) == 0) ? true : false;
+ return QTNodeCompare(a, b) == 0;
}
/*
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index cc2b4ac797..6c6786bc39 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -221,7 +221,7 @@ is_visible_fxid(FullTransactionId value, const pg_snapshot *snap)
res = bsearch(&value, snap->xip, snap->nxip, sizeof(FullTransactionId),
cmp_fxid);
/* if found, transaction is still in progress */
- return (res) ? false : true;
+ return !res;
}
#endif
else
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index e8c6cdde97..96fd9d2268 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -458,7 +458,7 @@ file_exists(const char *name)
AssertArg(name != NULL);
if (stat(name, &st) == 0)
- return S_ISDIR(st.st_mode) ? false : true;
+ return !S_ISDIR(st.st_mode);
else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
ereport(ERROR,
(errcode_for_file_access(),
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 <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
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
On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby <pryzby@telsasoft.com> 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 <pryzby@telsasoft.com> 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 <pryzbyj@telsasoft.com>
> 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);
> Page page = BufferGetPage(buffer);
> - bool is_leaf = (GistPageIsLeaf(page)) ? true : false;
> + bool is_leaf = GistPageIsLeaf(page);
> XLogRecPtr recptr;
> int i;
> bool is_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;
> + leaveOnLeft = sv->spl_rdatum_exists;
> }
>
> if (leaveOnLeft == false)
> diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
> index 0752fb38a9..5e3730201c 100644
> --- a/src/backend/access/hash/hash.c
> +++ b/src/backend/access/hash/hash.c
> @@ -816,7 +816,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf,
> XLogRecPtr recptr;
>
> xlrec.clear_dead_marking = clear_dead_marking;
> - xlrec.is_primary_bucket_page = (buf == bucket_buf) ? true : false;
> + xlrec.is_primary_bucket_page = buf == bucket_buf;
>
> XLogBeginInsert();
> XLogRegisterData((char *) &xlrec, SizeOfHashDelete);
> diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
> index d254a00b6a..83af8c1f67 100644
> --- a/src/backend/access/hash/hashinsert.c
> +++ b/src/backend/access/hash/hashinsert.c
> @@ -176,7 +176,7 @@ restart_insert:
> LockBuffer(buf, BUFFER_LOCK_UNLOCK);
>
> /* chain to a new overflow page */
> - buf = _hash_addovflpage(rel, metabuf, buf, (buf == bucket_buf) ? true : false);
> + buf = _hash_addovflpage(rel, metabuf, buf, buf == bucket_buf);
> page = BufferGetPage(buf);
>
> /* should fit now, given test above */
> diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
> index 404f2b6221..7397b6963f 100644
> --- a/src/backend/access/hash/hashovfl.c
> +++ b/src/backend/access/hash/hashovfl.c
> @@ -953,7 +953,7 @@ readpage:
> xl_hash_move_page_contents xlrec;
>
> xlrec.ntups = nitups;
> - xlrec.is_prim_bucket_same_wrt = (wbuf == bucket_buf) ? true : false;
> + xlrec.is_prim_bucket_same_wrt = wbuf == bucket_buf;
>
> XLogBeginInsert();
> XLogRegisterData((char *) &xlrec, SizeOfHashMovePageContents);
> diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
> index b730025356..eda10386b2 100644
> --- a/src/backend/access/hash/hashpage.c
> +++ b/src/backend/access/hash/hashpage.c
> @@ -1195,7 +1195,7 @@ _hash_splitbucket(Relation rel,
> all_tups_size = 0;
>
> /* chain to a new overflow page */
> - nbuf = _hash_addovflpage(rel, metabuf, nbuf, (nbuf == bucket_nbuf) ? true :
false);
> + nbuf = _hash_addovflpage(rel, metabuf, nbuf, nbuf == bucket_nbuf);
> npage = BufferGetPage(nbuf);
> nopaque = (HashPageOpaque) PageGetSpecialPointer(npage);
> }
> diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
> index d3c57cd16a..b72b03ea25 100644
> --- a/src/backend/access/heap/heapam_visibility.c
> +++ b/src/backend/access/heap/heapam_visibility.c
> @@ -1475,7 +1475,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
> * all relevant hint bits were just set moments ago).
> */
> if (!HeapTupleHeaderXminCommitted(tuple))
> - return HeapTupleHeaderXminInvalid(tuple) ? true : false;
> + return HeapTupleHeaderXminInvalid(tuple);
>
> /*
> * If the inserting transaction committed, but any deleting transaction
> diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
> index e14b9fa573..037d56132e 100644
> --- a/src/backend/access/spgist/spgscan.c
> +++ b/src/backend/access/spgist/spgscan.c
> @@ -860,7 +860,7 @@ redirect:
> page = BufferGetPage(buffer);
> TestForOldSnapshot(snapshot, index, page);
>
> - isnull = SpGistPageStoresNulls(page) ? true : false;
> + isnull = SpGistPageStoresNulls(page);
>
> if (SpGistPageIsLeaf(page))
> {
> diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
> index bf619d3a65..23fe1d85fd 100644
> --- a/src/backend/executor/spi.c
> +++ b/src/backend/executor/spi.c
> @@ -1037,7 +1037,7 @@ SPI_modifytuple(Relation rel, HeapTuple tuple, int natts, int *attnum,
> if (attnum[i] <= 0 || attnum[i] > numberOfAttributes)
> break;
> v[attnum[i] - 1] = Values[i];
> - n[attnum[i] - 1] = (Nulls && Nulls[i] == 'n') ? true : false;
> + n[attnum[i] - 1] = Nulls && Nulls[i] == 'n';
> }
>
> if (i == natts) /* no errors in *attnum */
> diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
> index 2da300e000..91b8ae6c51 100644
> --- a/src/backend/jit/jit.c
> +++ b/src/backend/jit/jit.c
> @@ -198,7 +198,7 @@ file_exists(const char *name)
> AssertArg(name != NULL);
>
> if (stat(name, &st) == 0)
> - return S_ISDIR(st.st_mode) ? false : true;
> + return !S_ISDIR(st.st_mode);
> else if (!(errno == ENOENT || errno == ENOTDIR))
> ereport(ERROR,
> (errcode_for_file_access(),
> diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
> index 41cbf328c4..3f14b5c1c4 100644
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -936,7 +936,7 @@ create_seqscan_path(PlannerInfo *root, RelOptInfo *rel,
> pathnode->pathtarget = rel->reltarget;
> pathnode->param_info = get_baserel_parampathinfo(root, rel,
>
required_outer);
> - pathnode->parallel_aware = parallel_workers > 0 ? true : false;
> + pathnode->parallel_aware = parallel_workers > 0;
> pathnode->parallel_safe = rel->consider_parallel;
> pathnode->parallel_workers = parallel_workers;
> pathnode->pathkeys = NIL; /* seqscan has unordered result */
> @@ -1057,7 +1057,7 @@ create_bitmap_heap_path(PlannerInfo *root,
> pathnode->path.pathtarget = rel->reltarget;
> pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
>
required_outer);
> - pathnode->path.parallel_aware = parallel_degree > 0 ? true : false;
> + pathnode->path.parallel_aware = parallel_degree > 0;
> pathnode->path.parallel_safe = rel->consider_parallel;
> pathnode->path.parallel_workers = parallel_degree;
> pathnode->path.pathkeys = NIL; /* always unordered */
> diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
> index ef118952c7..35b39ece07 100644
> --- a/src/backend/statistics/mcv.c
> +++ b/src/backend/statistics/mcv.c
> @@ -1772,7 +1772,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
> for (i = 0; i < mcvlist->nitems; i++)
> {
> int j;
> - bool match = (expr->useOr ? false : true);
> + bool match = !expr->useOr;
> MCVItem *item = &mcvlist->items[i];
>
> /*
> diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
> index a4be5fe513..41a6d4793c 100644
> --- a/src/backend/storage/file/buffile.c
> +++ b/src/backend/storage/file/buffile.c
> @@ -325,7 +325,7 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode)
>
> file = makeBufFileCommon(nfiles);
> file->files = files;
> - file->readOnly = (mode == O_RDONLY) ? true : false;
> + file->readOnly = mode == O_RDONLY;
> file->fileset = fileset;
> file->name = pstrdup(name);
>
> diff --git a/src/backend/tsearch/ts_parse.c b/src/backend/tsearch/ts_parse.c
> index d978c8850d..3ae0044dfb 100644
> --- a/src/backend/tsearch/ts_parse.c
> +++ b/src/backend/tsearch/ts_parse.c
> @@ -288,7 +288,7 @@ LexizeExec(LexizeData *ld, ParsedLex **correspondLexem)
> }
> }
>
> - ld->dictState.isend = (curVal->type == 0) ? true : false;
> + ld->dictState.isend = (curVal->type == 0);
> ld->dictState.getnext = false;
>
> res = (TSLexeme *) DatumGetPointer(FunctionCall4(&(dict->lexize),
> diff --git a/src/backend/utils/adt/bool.c b/src/backend/utils/adt/bool.c
> index fe11d1ae94..cd98f84270 100644
> --- a/src/backend/utils/adt/bool.c
> +++ b/src/backend/utils/adt/bool.c
> @@ -184,7 +184,7 @@ boolrecv(PG_FUNCTION_ARGS)
> int ext;
>
> ext = pq_getmsgbyte(buf);
> - PG_RETURN_BOOL((ext != 0) ? true : false);
> + PG_RETURN_BOOL(ext != 0);
> }
>
> /*
> diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
> index 4df8cc5abf..0f5f1208c9 100644
> --- a/src/backend/utils/adt/ruleutils.c
> +++ b/src/backend/utils/adt/ruleutils.c
> @@ -7997,14 +7997,14 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
> * appears simple since . has top precedence, unless parent is
> * T_FieldSelect itself!
> */
> - return (IsA(parentNode, FieldSelect) ? false : true);
> + return !IsA(parentNode, FieldSelect);
>
> case T_FieldStore:
>
> /*
> * treat like FieldSelect (probably doesn't matter)
> */
> - return (IsA(parentNode, FieldStore) ? false : true);
> + return !IsA(parentNode, FieldStore);
>
> case T_CoerceToDomain:
> /* maybe simple, check args */
> diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
> index 14d7343afa..906a686914 100644
> --- a/src/backend/utils/adt/tsquery_gist.c
> +++ b/src/backend/utils/adt/tsquery_gist.c
> @@ -109,7 +109,7 @@ gtsquery_same(PG_FUNCTION_ARGS)
> TSQuerySign b = PG_GETARG_TSQUERYSIGN(1);
> bool *result = (bool *) PG_GETARG_POINTER(2);
>
> - *result = (a == b) ? true : false;
> + *result = (a == b);
>
> PG_RETURN_POINTER(result);
> }
> diff --git a/src/backend/utils/adt/tsquery_util.c b/src/backend/utils/adt/tsquery_util.c
> index 7f936427b5..3dcc753e98 100644
> --- a/src/backend/utils/adt/tsquery_util.c
> +++ b/src/backend/utils/adt/tsquery_util.c
> @@ -186,7 +186,7 @@ QTNEq(QTNode *a, QTNode *b)
> if (!(sign == a->sign && sign == b->sign))
> return false;
>
> - return (QTNodeCompare(a, b) == 0) ? true : false;
> + return QTNodeCompare(a, b) == 0;
> }
>
> /*
> diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
> index cc2b4ac797..6c6786bc39 100644
> --- a/src/backend/utils/adt/xid8funcs.c
> +++ b/src/backend/utils/adt/xid8funcs.c
> @@ -221,7 +221,7 @@ is_visible_fxid(FullTransactionId value, const pg_snapshot *snap)
> res = bsearch(&value, snap->xip, snap->nxip, sizeof(FullTransactionId),
> cmp_fxid);
> /* if found, transaction is still in progress */
> - return (res) ? false : true;
> + return !res;
> }
> #endif
> else
> diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
> index e8c6cdde97..96fd9d2268 100644
> --- a/src/backend/utils/fmgr/dfmgr.c
> +++ b/src/backend/utils/fmgr/dfmgr.c
> @@ -458,7 +458,7 @@ file_exists(const char *name)
> AssertArg(name != NULL);
>
> if (stat(name, &st) == 0)
> - return S_ISDIR(st.st_mode) ? false : true;
> + return !S_ISDIR(st.st_mode);
> else if (!(errno == ENOENT || errno == ENOTDIR || errno == EACCES))
> ereport(ERROR,
> (errcode_for_file_access(),
>
>
If you are inclined to simplify all those ternary statements like
that, then you might also be interested in taking a look at lots of
similar code just using normal if/else (not ternary).
Try this hacky regex to expose some candidates (this works for me in
Visual Studio Code).
\s*([->*a-zA-Z_]+)\s*=\s*(true|false);.*\n\s*else\s*\n\s*\1*\s*=\s*(true|false);
IMO many of the examples that regex finds are best left alone (for readability).
OTOH there are still a few left that you might think would be better
to be simplified. e.g.
if (cube_cmp_v0(b1, b2) == 0)
*result = true;
else
*result = false;
--------
Kind Regards,
Peter Smith.
Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby <pryzby@telsasoft.com> 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
> On 19 Aug 2021, at 05:08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: >> On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby <pryzby@telsasoft.com> 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/
On Wed, Aug 18, 2021 at 11:08:57PM -0400, Tom Lane wrote: > Peter Smith <smithpb2250@gmail.com> writes: > > On Thu, Aug 19, 2021 at 4:29 AM Justin Pryzby <pryzby@telsasoft.com> 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 <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
At Thu, 9 Sep 2021 13:28:54 +0900, Michael Paquier <michael@paquier.xyz> 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�187:#define strtobool(x) ((*(x) == 't') ? true : false) ./backend/tsearch/wparser_def.c�1859: return (item && (item->flags & A_BINGO)) ? true : false; These are candidates to fix. ./backend/tsearch/ts_utils.c�145: sizeof(char *), pg_qsort_strcmp)) ? true : false; This is a part of the following expression. > return (s->stop && s->len > 0 && > bsearch(&key, s->stop, s->len, > sizeof(char *), pg_qsort_strcmp)) ? true : false; So this is also a candidate. Also found !f(eval) equivalents. ./backend/access/gist/gistsplit.c�424: sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true; ./backend/access/gist/gistsplit.c�425: sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true; ./backend/access/gist/gistsplit.c�424: sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true; ./backend/access/gist/gistsplit.c�425: sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true; ./backend/access/gist/gistsplit.c�454: sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true; ./backend/access/gist/gistsplit.c�455: sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true; ./backend/commands/tablecmds.c�7466: newDefault == NULL ? false : true); ./backend/executor/spi.c�146: _SPI_current->atomic = (options & SPI_OPT_NONATOMIC ? false : true); ./backend/executor/nodeResult.c�198: resstate->rs_checkqual = (node->resconstantqual == NULL) ? false : true; ./backend/executor/nodeResult.c�263: node->rs_checkqual = (node->resconstantqual == NULL) ? false : true; ./backend/statistics/mcv.c�1622: memset(matches, (is_or) ? false : true, ./backend/tsearch/spell.c�1708: ? false : true; ./interfaces/ecpg/ecpglib/execute.c�124: string = string ? false : true; ./interfaces/ecpg/ecpglib/prepare.c�113: string = string ? false : true; ./interfaces/ecpg/ecpglib/data.c�959: string = string ? false : true; (Note: the "string" is a bool) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
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
Attachment
On Sat, Sep 11, 2021 at 2:44 PM Michael Paquier <michael@paquier.xyz> 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/
Attachment
On Thu, Oct 7, 2021 at 11:44 AM Justin Pryzby <pryzby@telsasoft.com> 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 <michael@paquier.xyz> 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.GS831@telsasoft.com Good. Thank you for the information! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On Thu, Oct 07, 2021 at 01:27:34PM +0900, Masahiko Sawada wrote: > On Thu, Oct 7, 2021 at 11:44 AM Justin Pryzby <pryzby@telsasoft.com> 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
Attachment
On Thu, Oct 7, 2021 at 1:37 PM Michael Paquier <michael@paquier.xyz> 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 <pryzby@telsasoft.com> 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/
Attachment
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
Attachment
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
Attachment
On Mon, Oct 11, 2021 at 9:45 AM Michael Paquier <michael@paquier.xyz> 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/