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

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

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

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.

From 2534ff46830ab505a6a2079a1b6c0f8a1e9b2b8b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 28 Apr 2021 14:12:49 -0500
Subject: [PATCH 1/2] Avoid double parens

git grep -l '\<if (([^(]*))' '*.c'
---
 contrib/amcheck/verify_heapam.c          | 6 +++---
 contrib/ltree/ltree_io.c                 | 6 +++---
 src/backend/access/transam/xact.c        | 6 +++---
 src/backend/commands/tablecmds.c         | 2 +-
 src/backend/nodes/nodeFuncs.c            | 4 ++--
 src/backend/replication/logical/decode.c | 2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index a3caee7cdd..cd275bd45d 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -389,13 +389,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 													   &vmbuffer);
 			if (skip_option == SKIP_PAGES_ALL_FROZEN)
 			{
-				if ((mapbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+				if (mapbits & VISIBILITYMAP_ALL_FROZEN)
 					continue;
 			}
 
 			if (skip_option == SKIP_PAGES_ALL_VISIBLE)
 			{
-				if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
+				if (mapbits & VISIBILITYMAP_ALL_VISIBLE)
 					continue;
 			}
 		}
@@ -692,7 +692,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)
 			report_corruption(ctx,
 							  psprintf("tuple data should begin at byte %u, but actually begins at byte %u (%u attributes, has nulls)",
 									   expected_hoff, ctx->tuphdr->t_hoff, ctx->natts));
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index 15115cb29f..b75f35d5b5 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -661,17 +661,17 @@ deparse_lquery(const lquery *in)
 				}
 				memcpy(ptr, curtlevel->name, curtlevel->len);
 				ptr += curtlevel->len;
-				if ((curtlevel->flag & LVAR_SUBLEXEME))
+				if (curtlevel->flag & LVAR_SUBLEXEME)
 				{
 					*ptr = '%';
 					ptr++;
 				}
-				if ((curtlevel->flag & LVAR_INCASE))
+				if (curtlevel->flag & LVAR_INCASE)
 				{
 					*ptr = '@';
 					ptr++;
 				}
-				if ((curtlevel->flag & LVAR_ANYEND))
+				if (curtlevel->flag & LVAR_ANYEND)
 				{
 					*ptr = '*';
 					ptr++;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index da8a460722..9c13a7c589 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2452,7 +2452,7 @@ PrepareTransaction(void)
 	 * cases, such as a temp table created and dropped all within the
 	 * transaction.  That seems to require much more bookkeeping though.
 	 */
-	if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+	if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
@@ -5565,7 +5565,7 @@ XactLogCommitRecord(TimestampTz commit_time,
 		xl_xinfo.xinfo |= XACT_COMPLETION_UPDATE_RELCACHE_FILE;
 	if (forceSyncCommit)
 		xl_xinfo.xinfo |= XACT_COMPLETION_FORCE_SYNC_COMMIT;
-	if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+	if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
 		xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
 
 	/*
@@ -5716,7 +5716,7 @@ XactLogAbortRecord(TimestampTz abort_time,
 
 	xlrec.xact_time = abort_time;
 
-	if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+	if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
 		xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
 
 	if (nsubxacts > 0)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4e23c7fce5..39180999fd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16171,7 +16171,7 @@ PreCommit_on_commit_actions(void)
 				 * relations, we can skip truncating ON COMMIT DELETE ROWS
 				 * tables, as they must still be empty.
 				 */
-				if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+				if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
 					oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
 				break;
 			case ONCOMMIT_DROP:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index ff3dcc7b18..ae3364dfdc 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2390,7 +2390,7 @@ query_tree_walker(Query *query,
 	 * don't contain actual expressions. However they do contain OIDs which
 	 * may be needed by dependency walkers etc.
 	 */
-	if ((flags & QTW_EXAMINE_SORTGROUP))
+	if (flags & QTW_EXAMINE_SORTGROUP)
 	{
 		if (walker((Node *) query->groupClause, context))
 			return true;
@@ -3328,7 +3328,7 @@ query_tree_mutator(Query *query,
 	 * may be of interest to some mutators.
 	 */
 
-	if ((flags & QTW_EXAMINE_SORTGROUP))
+	if (flags & QTW_EXAMINE_SORTGROUP)
 	{
 		MUTATE(query->groupClause, query->groupClause, List *);
 		MUTATE(query->windowClause, query->windowClause, List *);
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 453efc51e1..34a36b22f8 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -337,7 +337,7 @@ DecodeXactOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 					ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
 													  buf->origptr);
 				}
-				else if ((!ctx->fast_forward))
+				else if (!ctx->fast_forward)
 					ReorderBufferImmediateInvalidation(ctx->reorder,
 													   invals->nmsgs,
 													   invals->msgs);
-- 
2.17.0

From 16773708a7ab2c995fa3d7e1bba1b3626468a8d0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 8 May 2021 08:12:46 -0500
Subject: [PATCH 2/2] tablefunc.c: Fix comment

Mentioned at: https://www.postgresql.org/message-id/CAJRYxuKmMkgHFKjuYPxThdT5Mjg%2Bg-nH5szYbS8UoVsQXzd95A%40mail.gmail.com
---
 contrib/tablefunc/tablefunc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 779bd4415e..52b272f298 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -1480,7 +1480,7 @@ validateConnectbyTupleDesc(TupleDesc td, bool show_branch, bool show_serial)
 						"fifth column must be type %s",
 						format_type_be(INT4OID))));
 
-	/* check that the type of the fifth column is INT4 */
+	/* check that the type of the fourth column is INT4 */
 	if (!show_branch && show_serial &&
 		TupleDescAttr(td, 3)->atttypid != INT4OID)
 		ereport(ERROR,
-- 
2.17.0

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.

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

From 51dab66eb3cbeccfa5bad0f1b8a26b94523edb65 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 28 Apr 2021 14:12:49 -0500
Subject: [PATCH 1/2] Avoid double parens

git grep -l '\<if (([^(]*))' '*.c'
---
 contrib/amcheck/verify_heapam.c          | 6 +++---
 contrib/ltree/ltree_io.c                 | 6 +++---
 src/backend/access/transam/xact.c        | 6 +++---
 src/backend/commands/tablecmds.c         | 2 +-
 src/backend/nodes/nodeFuncs.c            | 4 ++--
 src/backend/replication/logical/decode.c | 2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 173f99d787..0ac52b6ba2 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -406,13 +406,13 @@ verify_heapam(PG_FUNCTION_ARGS)
 													   &vmbuffer);
 			if (skip_option == SKIP_PAGES_ALL_FROZEN)
 			{
-				if ((mapbits & VISIBILITYMAP_ALL_FROZEN) != 0)
+				if (mapbits & VISIBILITYMAP_ALL_FROZEN)
 					continue;
 			}
 
 			if (skip_option == SKIP_PAGES_ALL_VISIBLE)
 			{
-				if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
+				if (mapbits & VISIBILITYMAP_ALL_VISIBLE)
 					continue;
 			}
 		}
@@ -690,7 +690,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)
 			report_corruption(ctx,
 							  psprintf("tuple data should begin at byte %u, but actually begins at byte %u (%u attributes, has nulls)",
 									   expected_hoff, ctx->tuphdr->t_hoff, ctx->natts));
diff --git a/contrib/ltree/ltree_io.c b/contrib/ltree/ltree_io.c
index 15115cb29f..b75f35d5b5 100644
--- a/contrib/ltree/ltree_io.c
+++ b/contrib/ltree/ltree_io.c
@@ -661,17 +661,17 @@ deparse_lquery(const lquery *in)
 				}
 				memcpy(ptr, curtlevel->name, curtlevel->len);
 				ptr += curtlevel->len;
-				if ((curtlevel->flag & LVAR_SUBLEXEME))
+				if (curtlevel->flag & LVAR_SUBLEXEME)
 				{
 					*ptr = '%';
 					ptr++;
 				}
-				if ((curtlevel->flag & LVAR_INCASE))
+				if (curtlevel->flag & LVAR_INCASE)
 				{
 					*ptr = '@';
 					ptr++;
 				}
-				if ((curtlevel->flag & LVAR_ANYEND))
+				if (curtlevel->flag & LVAR_ANYEND)
 				{
 					*ptr = '*';
 					ptr++;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 387f80419a..1ba9dbf966 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2417,7 +2417,7 @@ PrepareTransaction(void)
 	 * cases, such as a temp table created and dropped all within the
 	 * transaction.  That seems to require much more bookkeeping though.
 	 */
-	if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+	if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
@@ -5530,7 +5530,7 @@ XactLogCommitRecord(TimestampTz commit_time,
 		xl_xinfo.xinfo |= XACT_COMPLETION_UPDATE_RELCACHE_FILE;
 	if (forceSyncCommit)
 		xl_xinfo.xinfo |= XACT_COMPLETION_FORCE_SYNC_COMMIT;
-	if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+	if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
 		xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
 
 	/*
@@ -5681,7 +5681,7 @@ XactLogAbortRecord(TimestampTz abort_time,
 
 	xlrec.xact_time = abort_time;
 
-	if ((xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK))
+	if (xactflags & XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK)
 		xl_xinfo.xinfo |= XACT_XINFO_HAS_AE_LOCKS;
 
 	if (nsubxacts > 0)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dbee6ae199..e018cdfd9e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16309,7 +16309,7 @@ PreCommit_on_commit_actions(void)
 				 * relations, we can skip truncating ON COMMIT DELETE ROWS
 				 * tables, as they must still be empty.
 				 */
-				if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
+				if (MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)
 					oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
 				break;
 			case ONCOMMIT_DROP:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index ff3dcc7b18..ae3364dfdc 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2390,7 +2390,7 @@ query_tree_walker(Query *query,
 	 * don't contain actual expressions. However they do contain OIDs which
 	 * may be needed by dependency walkers etc.
 	 */
-	if ((flags & QTW_EXAMINE_SORTGROUP))
+	if (flags & QTW_EXAMINE_SORTGROUP)
 	{
 		if (walker((Node *) query->groupClause, context))
 			return true;
@@ -3328,7 +3328,7 @@ query_tree_mutator(Query *query,
 	 * may be of interest to some mutators.
 	 */
 
-	if ((flags & QTW_EXAMINE_SORTGROUP))
+	if (flags & QTW_EXAMINE_SORTGROUP)
 	{
 		MUTATE(query->groupClause, query->groupClause, List *);
 		MUTATE(query->windowClause, query->windowClause, List *);
diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c
index 2874dc0612..fc9a0d67c9 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -337,7 +337,7 @@ DecodeXactOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
 					ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
 													  buf->origptr);
 				}
-				else if ((!ctx->fast_forward))
+				else if (!ctx->fast_forward)
 					ReorderBufferImmediateInvalidation(ctx->reorder,
 													   invals->nmsgs,
 													   invals->msgs);
-- 
2.17.0

From 04fbf1de8afee69c1ad843c0d030c678ae244bf1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 7 May 2021 08:16:51 -0500
Subject: [PATCH 2/2] Avoid verbose ternary operator with expressions which are
 already boolean

---
 contrib/intarray/_int_tool.c                | 2 +-
 contrib/ltree/ltree_gist.c                  | 2 +-
 contrib/sepgsql/selinux.c                   | 2 +-
 src/backend/access/gin/gindatapage.c        | 2 +-
 src/backend/access/gin/ginutil.c            | 2 +-
 src/backend/access/gist/gist.c              | 2 +-
 src/backend/access/gist/gistsplit.c         | 4 ++--
 src/backend/access/hash/hash.c              | 2 +-
 src/backend/access/hash/hashinsert.c        | 2 +-
 src/backend/access/hash/hashovfl.c          | 2 +-
 src/backend/access/hash/hashpage.c          | 2 +-
 src/backend/access/heap/heapam_visibility.c | 2 +-
 src/backend/access/spgist/spgscan.c         | 2 +-
 src/backend/executor/spi.c                  | 2 +-
 src/backend/jit/jit.c                       | 2 +-
 src/backend/optimizer/util/pathnode.c       | 4 ++--
 src/backend/statistics/mcv.c                | 2 +-
 src/backend/storage/file/buffile.c          | 2 +-
 src/backend/tsearch/ts_parse.c              | 2 +-
 src/backend/utils/adt/bool.c                | 2 +-
 src/backend/utils/adt/ruleutils.c           | 4 ++--
 src/backend/utils/adt/tsquery_gist.c        | 2 +-
 src/backend/utils/adt/tsquery_util.c        | 2 +-
 src/backend/utils/adt/xid8funcs.c           | 2 +-
 src/backend/utils/fmgr/dfmgr.c              | 2 +-
 25 files changed, 28 insertions(+), 28 deletions(-)

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..6264d7273c 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..fbccf3d038 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..6d2d71be32 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..eb3810494f 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..fe9f0df20b 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..b312af57e1 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..159646c7c3 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 401a1a8343..3fdcb40a88 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -864,7 +864,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 a8d7fe6dab..a5aec7ba7d 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 a53850b370..81d9f25a46 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 ff3aa67cde..b673150dbe 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -336,7 +336,7 @@ BufFileOpenFileSet(FileSet *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 8ff4e5dc07..1fb0b7b098 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8005,14 +8005,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..b3dd4f4be0 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(),
-- 
2.17.0

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

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

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

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

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]/messages/by-id/20210906001110.GF26465@telsasoft.com 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]/messages/by-id/20210909.141450.11969674682374713.horikyota.ntt@gmail.com. I've attached the patch to remove them.

Regards,

[1]: /messages/by-id/20210906001110.GF26465@telsasoft.com
[2]: /messages/by-id/20210909.141450.11969674682374713.horikyota.ntt@gmail.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

diff --git a/contrib/ltree/ltree_op.c b/contrib/ltree/ltree_op.c
index 778dbf1e98..da1db5fcd2 100644
--- a/contrib/ltree/ltree_op.c
+++ b/contrib/ltree/ltree_op.c
@@ -91,42 +91,42 @@ Datum
 ltree_lt(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res < 0) ? true : false);
+	PG_RETURN_BOOL(res < 0);
 }
 
 Datum
 ltree_le(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res <= 0) ? true : false);
+	PG_RETURN_BOOL(res <= 0);
 }
 
 Datum
 ltree_eq(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res == 0) ? true : false);
+	PG_RETURN_BOOL(res == 0);
 }
 
 Datum
 ltree_ge(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res >= 0) ? true : false);
+	PG_RETURN_BOOL(res >= 0);
 }
 
 Datum
 ltree_gt(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res > 0) ? true : false);
+	PG_RETURN_BOOL(res > 0);
 }
 
 Datum
 ltree_ne(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res != 0) ? true : false);
+	PG_RETURN_BOOL(res != 0);
 }
 
 Datum
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index abf08b7a2f..e6aa52bb4b 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -184,7 +184,7 @@
  */
 #define atoui(x)  ((unsigned int) strtoul((x), NULL, 10))
 
-#define strtobool(x)  ((*(x) == 't') ? true : false)
+#define strtobool(x)  ((*(x) == 't'))
 
 #define nullable_string(token,length)  \
 	((length) == 0 ? NULL : debackslash(token, length))
diff --git a/src/backend/tsearch/ts_utils.c b/src/backend/tsearch/ts_utils.c
index ed16a2e25a..9539cf1326 100644
--- a/src/backend/tsearch/ts_utils.c
+++ b/src/backend/tsearch/ts_utils.c
@@ -142,5 +142,5 @@ searchstoplist(StopList *s, char *key)
 {
 	return (s->stop && s->len > 0 &&
 			bsearch(&key, s->stop, s->len,
-					sizeof(char *), pg_qsort_strcmp)) ? true : false;
+					sizeof(char *), pg_qsort_strcmp));
 }
diff --git a/src/backend/tsearch/wparser_def.c b/src/backend/tsearch/wparser_def.c
index 559dff6355..537c701cc7 100644
--- a/src/backend/tsearch/wparser_def.c
+++ b/src/backend/tsearch/wparser_def.c
@@ -1856,7 +1856,7 @@ TParserGet(TParser *prs)
 		}
 	}
 
-	return (item && (item->flags & A_BINGO)) ? true : false;
+	return (item && (item->flags & A_BINGO));
 }
 
 Datum

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:
/messages/by-id/20210924215827.GS831@telsasoft.com

Michael seems prefer writing (() != 0) in more cases than other people, so
didn't care for that patch.
/messages/by-id/577206.1620628321@sss.pgh.pa.us

--
Justin

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:
/messages/by-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

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/

diff --git a/contrib/ltree/ltree_op.c b/contrib/ltree/ltree_op.c
index 778dbf1e98..da1db5fcd2 100644
--- a/contrib/ltree/ltree_op.c
+++ b/contrib/ltree/ltree_op.c
@@ -91,42 +91,42 @@ Datum
 ltree_lt(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res < 0) ? true : false);
+	PG_RETURN_BOOL(res < 0);
 }
 
 Datum
 ltree_le(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res <= 0) ? true : false);
+	PG_RETURN_BOOL(res <= 0);
 }
 
 Datum
 ltree_eq(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res == 0) ? true : false);
+	PG_RETURN_BOOL(res == 0);
 }
 
 Datum
 ltree_ge(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res >= 0) ? true : false);
+	PG_RETURN_BOOL(res >= 0);
 }
 
 Datum
 ltree_gt(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res > 0) ? true : false);
+	PG_RETURN_BOOL(res > 0);
 }
 
 Datum
 ltree_ne(PG_FUNCTION_ARGS)
 {
 	RUNCMP;
-	PG_RETURN_BOOL((res != 0) ? true : false);
+	PG_RETURN_BOOL(res != 0);
 }
 
 Datum
diff --git a/src/backend/access/gist/gistsplit.c b/src/backend/access/gist/gistsplit.c
index 853ebc387b..1c5d4e9ca2 100644
--- a/src/backend/access/gist/gistsplit.c
+++ b/src/backend/access/gist/gistsplit.c
@@ -421,8 +421,8 @@ gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVec
 	 * Prepare spl_ldatum/spl_rdatum/spl_ldatum_exists/spl_rdatum_exists in
 	 * case we are doing a secondary split (see comments in gist.h).
 	 */
-	sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true;
-	sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true;
+	sv->spl_ldatum_exists = !(v->spl_lisnull[attno]);
+	sv->spl_rdatum_exists = !(v->spl_risnull[attno]);
 	sv->spl_ldatum = v->spl_lattr[attno];
 	sv->spl_rdatum = v->spl_rattr[attno];
 
@@ -451,8 +451,8 @@ gistUserPicksplit(Relation r, GistEntryVector *entryvec, int attno, GistSplitVec
 		 * Reinit GIST_SPLITVEC. Although these fields are not used by
 		 * genericPickSplit(), set them up for further processing
 		 */
-		sv->spl_ldatum_exists = (v->spl_lisnull[attno]) ? false : true;
-		sv->spl_rdatum_exists = (v->spl_risnull[attno]) ? false : true;
+		sv->spl_ldatum_exists = !(v->spl_lisnull[attno]);
+		sv->spl_rdatum_exists = !(v->spl_risnull[attno]);
 		sv->spl_ldatum = v->spl_lattr[attno];
 		sv->spl_rdatum = v->spl_rattr[attno];
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f0ad23161c..1cbbf620bb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7463,7 +7463,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
 	 * operation when the user asked for a drop.
 	 */
 	RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, false,
-					  newDefault == NULL ? false : true);
+					  newDefault != NULL);
 
 	if (newDefault)
 	{
diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c
index 0946af0a54..a8d308c660 100644
--- a/src/backend/executor/nodeResult.c
+++ b/src/backend/executor/nodeResult.c
@@ -195,7 +195,7 @@ ExecInitResult(Result *node, EState *estate, int eflags)
 	resstate->ps.ExecProcNode = ExecResult;
 
 	resstate->rs_done = false;
-	resstate->rs_checkqual = (node->resconstantqual == NULL) ? false : true;
+	resstate->rs_checkqual = (node->resconstantqual != NULL);
 
 	/*
 	 * Miscellaneous initialization
@@ -260,7 +260,7 @@ void
 ExecReScanResult(ResultState *node)
 {
 	node->rs_done = false;
-	node->rs_checkqual = (node->resconstantqual == NULL) ? false : true;
+	node->rs_checkqual = (node->resconstantqual != NULL);
 
 	/*
 	 * If chgParam of subnode is not null then plan will be re-scanned by
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index abf08b7a2f..e6aa52bb4b 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -184,7 +184,7 @@
  */
 #define atoui(x)  ((unsigned int) strtoul((x), NULL, 10))
 
-#define strtobool(x)  ((*(x) == 't') ? true : false)
+#define strtobool(x)  ((*(x) == 't'))
 
 #define nullable_string(token,length)  \
 	((length) == 0 ? NULL : debackslash(token, length))
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 35b39ece07..b350fc5f7b 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1619,8 +1619,7 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses,
 	Assert(mcvlist->nitems <= STATS_MCVLIST_MAX_ITEMS);
 
 	matches = palloc(sizeof(bool) * mcvlist->nitems);
-	memset(matches, (is_or) ? false : true,
-		   sizeof(bool) * mcvlist->nitems);
+	memset(matches, !is_or,  sizeof(bool) * mcvlist->nitems);
 
 	/*
 	 * Loop through the list of clauses, and for each of them evaluate all the
diff --git a/src/backend/tsearch/ts_utils.c b/src/backend/tsearch/ts_utils.c
index ed16a2e25a..9539cf1326 100644
--- a/src/backend/tsearch/ts_utils.c
+++ b/src/backend/tsearch/ts_utils.c
@@ -142,5 +142,5 @@ searchstoplist(StopList *s, char *key)
 {
 	return (s->stop && s->len > 0 &&
 			bsearch(&key, s->stop, s->len,
-					sizeof(char *), pg_qsort_strcmp)) ? true : false;
+					sizeof(char *), pg_qsort_strcmp));
 }
diff --git a/src/backend/tsearch/wparser_def.c b/src/backend/tsearch/wparser_def.c
index 559dff6355..537c701cc7 100644
--- a/src/backend/tsearch/wparser_def.c
+++ b/src/backend/tsearch/wparser_def.c
@@ -1856,7 +1856,7 @@ TParserGet(TParser *prs)
 		}
 	}
 
-	return (item && (item->flags & A_BINGO)) ? true : false;
+	return (item && (item->flags & A_BINGO));
 }
 
 Datum

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

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

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/