Horton

Thread

Commits

  1. Clean up more code using "(expr) ? true : false"

  2. Clean up some code using "(expr) ? true : false"

  1. strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-04-28T18:29:36Z

    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);

  2. Re: strange case of "if ((a & b))"

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-28T18:40:09Z

    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

  3. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-05-10T06:25:08Z

    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

  4. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-05-10T06:27:13Z

    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?

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

  5. Re: strange case of "if ((a & b))"

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-10T06:32:01Z

    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

  6. Re: strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-25T02:31:11Z

    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.

  7. Re: strange case of "if ((a & b))"

    Bruce Momjian <bruce@momjian.us> — 2021-08-18T18:02:22Z

    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.

  8. Re: strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-08-18T18:28:56Z

    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(),

  9. Re: strange case of "if ((a & b))"

    Bruce Momjian <bruce@momjian.us> — 2021-08-18T19:15:21Z

    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.

  10. Re: strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-08-18T19:23:18Z

    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

  11. Re: strange case of "if ((a & b))"

    Peter Smith <smithpb2250@gmail.com> — 2021-08-19T00:59:39Z

    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

  12. Re: strange case of "if ((a & b))"

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-19T03:08:57Z

    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

  13. Re: strange case of "if ((a & b))"

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-19T07:34:35Z

    > 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/

  14. Re: strange case of "if ((a & b))"

    Bruce Momjian <bruce@momjian.us> — 2021-08-19T18:26:36Z

    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.

  15. Re: strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-09-06T00:11:10Z

    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);
> 
> 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

  16. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-09-07T05:59:58Z

    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

  17. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-09-09T04:28:54Z

    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

  18. Re: strange case of "if ((a & b))"

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-09T05:14:50Z

    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.c187:#define strtobool(x)  ((*(x) == 't') ? true : false)
./backend/tsearch/wparser_def.c1859:	return (item && (item->flags & A_BINGO)) ? true : false;

These are candidates to fix.

./backend/tsearch/ts_utils.c145:					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.c424:	sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true;
./backend/access/gist/gistsplit.c425:	sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true;
./backend/access/gist/gistsplit.c424:	sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true;
./backend/access/gist/gistsplit.c425:	sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true;
./backend/access/gist/gistsplit.c454:		sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true;
./backend/access/gist/gistsplit.c455:		sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true;
./backend/commands/tablecmds.c7466:					  newDefault == NULL ? false : true);
./backend/executor/spi.c146:	_SPI_current->atomic = (options & SPI_OPT_NONATOMIC ? false : true);
./backend/executor/nodeResult.c198:	resstate->rs_checkqual = (node->resconstantqual == NULL) ? false : true;
./backend/executor/nodeResult.c263:	node->rs_checkqual = (node->resconstantqual == NULL) ? false : true;
./backend/statistics/mcv.c1622:	memset(matches, (is_or) ? false : true,
./backend/tsearch/spell.c1708:						? false : true;

./interfaces/ecpg/ecpglib/execute.c124:			string = string ? false : true;
./interfaces/ecpg/ecpglib/prepare.c113:			string = string ? false : true;
./interfaces/ecpg/ecpglib/data.c959:						string = string ? false : true;
(Note: the "string" is a bool)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  19. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-09-11T05:44:11Z

    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

  20. Re: strange case of "if ((a & b))"

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-07T02:18:24Z

    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/

  21. Re: strange case of "if ((a & b))"

    Justin Pryzby <pryzby@telsasoft.com> — 2021-10-07T02:44:30Z

    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 ;)

> 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

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.1620628321@sss.pgh.pa.us

-- 
Justin

  22. Re: strange case of "if ((a & b))"

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-07T04:27:34Z

    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/

  23. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-10-07T04:36:57Z

    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

  24. Re: strange case of "if ((a & b))"

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-07T06:24:53Z

    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/

  25. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-10-07T07:49:10Z

    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

  26. Re: strange case of "if ((a & b))"

    Michael Paquier <michael@paquier.xyz> — 2021-10-11T00:45:26Z

    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

  27. Re: strange case of "if ((a & b))"

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-10-11T01:21:28Z

    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/