Re: strange case of "if ((a & b))" - Mailing list pgsql-hackers

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