Thread

Commits

Avoid using list_length() to test for empty list.
- efd0c16becbf 16.0 landed

Propose a new function - list_is_empty

Peter Smith <smithpb2250@gmail.com> — 2022-08-16T01:19:47Z

During a recent code review I was going to suggest that some new code
would be more readable if the following:
if (list_length(alist) == 0) ...

was replaced with:
if (list_is_empty(alist)) ...

but then I found that actually no such function exists.

~~~

Searching the PG source found many cases using all kinds of
inconsistent ways to test for empty Lists:
e.g.1 if (list_length(alist) > 0)
e.g.2 if (list_length(alist) == 0)
e.g.3 if (list_length(alist) != 0)
e.g.4 if (list_length(alist) >= 1)
e.g.5 if (list_length(alist) < 1)

Of course, all of them work OK as-is, but by using list_is_empty all
those can be made consistent and often also more readable as to the
code intent.

Patch 0001 adds a new function 'list_is_empty'.
Patch 0002 makes use of it.

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Propose a new function - list_is_empty

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-16T01:27:56Z

Peter Smith <smithpb2250@gmail.com> writes:
> During a recent code review I was going to suggest that some new code
> would be more readable if the following:
> if (list_length(alist) == 0) ...

> was replaced with:
> if (list_is_empty(alist)) ...

> but then I found that actually no such function exists.

That's because the *correct* way to write it is either "alist == NIL"
or just "!alist".  I don't think we need yet another way to spell
that, and I'm entirely not on board with replacing either of those
idioms.  But if you want to get rid of overcomplicated uses of
list_length() in favor of one of those spellings, have at it.

			regards, tom lane

Re: Propose a new function - list_is_empty

Peter Smith <smithpb2250@gmail.com> — 2022-08-16T01:39:25Z

On Tue, Aug 16, 2022 at 11:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > During a recent code review I was going to suggest that some new code
> > would be more readable if the following:
> > if (list_length(alist) == 0) ...
>
> > was replaced with:
> > if (list_is_empty(alist)) ...
>
> > but then I found that actually no such function exists.
>
> That's because the *correct* way to write it is either "alist == NIL"
> or just "!alist".  I don't think we need yet another way to spell
> that, and I'm entirely not on board with replacing either of those
> idioms.  But if you want to get rid of overcomplicated uses of
> list_length() in favor of one of those spellings, have at it.
>

Thanks for your advice.

Yes, I saw that NIL is the definition of an empty list - that's how I
implemented list_is_empty.

OK, I'll ditch the function idea and just look at de-complicating
those existing empty List checks.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Propose a new function - list_is_empty

Peter Smith <smithpb2250@gmail.com> — 2022-08-16T05:29:29Z

On Tue, Aug 16, 2022 at 11:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > During a recent code review I was going to suggest that some new code
> > would be more readable if the following:
> > if (list_length(alist) == 0) ...
>
> > was replaced with:
> > if (list_is_empty(alist)) ...
>
> > but then I found that actually no such function exists.
>
> That's because the *correct* way to write it is either "alist == NIL"
> or just "!alist".  I don't think we need yet another way to spell
> that, and I'm entirely not on board with replacing either of those
> idioms.  But if you want to get rid of overcomplicated uses of
> list_length() in favor of one of those spellings, have at it.

Done, and tested OK with make check-world.

PSA.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Re: Propose a new function - list_is_empty

Daniel Gustafsson <daniel@yesql.se> — 2022-08-16T08:39:36Z

> On 16 Aug 2022, at 07:29, Peter Smith <smithpb2250@gmail.com> wrote:
> On Tue, Aug 16, 2022 at 11:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> if you want to get rid of overcomplicated uses of
>> list_length() in favor of one of those spellings, have at it.
> 
> Done, and tested OK with make check-world.

I think these are nice cleanups to simplify and streamline the code, just a few
small comments from reading the patch:

 	/* If no subcommands, don't collect */
-	if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0)
+	if (currentEventTriggerState->currentCommand->d.alterTable.subcmds)
Here the current coding gives context about the data structure used for the
subcmds member which is now lost.  I don't mind the change but rewording the
comment above to indicate that subcmds is a list would be good IMHO.


-	build_expressions = (list_length(stxexprs) > 0);
+	build_expressions = stxexprs != NIL;
Might be personal taste, but I think the parenthesis should be kept here as a
visual aid for the reader.


-	Assert(list_length(publications) > 0);
+	Assert(publications);
The more common (and clearer IMO) pattern would be Assert(publications != NIL);
I think.  The same applies for a few hunks in the patch.


-	Assert(clauses != NIL);
-	Assert(list_length(clauses) >= 1);
+	Assert(clauses);
Just removing the list_length() assertion would be enough here.


makeIndexArray() in jsonpath_gram.y has another Assert(list_length(list) > 0);
construction as well.  The other I found is in create_groupingsets_path() but
there I think it makes sense to keep the current coding based on the assertion
just prior to it being very similar and requiring list_length().

--
Daniel Gustafsson		https://vmware.com/

Re: Propose a new function - list_is_empty

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-16T13:37:51Z

Daniel Gustafsson <daniel@yesql.se> writes:
> I think these are nice cleanups to simplify and streamline the code, just a few
> small comments from reading the patch:

>  	/* If no subcommands, don't collect */
> -	if (list_length(currentEventTriggerState->currentCommand->d.alterTable.subcmds) != 0)
> +	if (currentEventTriggerState->currentCommand->d.alterTable.subcmds)
> Here the current coding gives context about the data structure used for the
> subcmds member which is now lost.  I don't mind the change but rewording the
> comment above to indicate that subcmds is a list would be good IMHO.

I think testing for equality to NIL is better where that's a concern.

> Might be personal taste, but I think the parenthesis should be kept here as a
> visual aid for the reader.

+1

			regards, tom lane

Re: Propose a new function - list_is_empty

Robert Haas <robertmhaas@gmail.com> — 2022-08-16T13:47:56Z

On Mon, Aug 15, 2022 at 9:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Smith <smithpb2250@gmail.com> writes:
> > During a recent code review I was going to suggest that some new code
> > would be more readable if the following:
> > if (list_length(alist) == 0) ...
>
> > was replaced with:
> > if (list_is_empty(alist)) ...
>
> > but then I found that actually no such function exists.
>
> That's because the *correct* way to write it is either "alist == NIL"
> or just "!alist".

I think the alist == NIL (or alist != NIL) style often makes the code
easier to read. I recommend we standardize on that one.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Propose a new function - list_is_empty

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-16T14:03:57Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Aug 15, 2022 at 9:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That's because the *correct* way to write it is either "alist == NIL"
>> or just "!alist".

> I think the alist == NIL (or alist != NIL) style often makes the code
> easier to read. I recommend we standardize on that one.

I have a general preference for comparing to NIL because (as Daniel
noted nearby) it reminds you of what data type you're dealing with.
However, I'm not up for trying to forbid the bare-boolean-test style
altogether.  It'd be near impossible to find all the instances;
besides which we don't insist that other pointer checks be written
as explicit comparisons to NULL --- we do whichever of those seems
clearest in context.  So I'm happy for this patch to leave either
of those existing usages alone.  I agree though that while simplifying
list_length() calls, I'd lean to using explicit comparisons to NIL.

			regards, tom lane

Re: Propose a new function - list_is_empty

Daniel Gustafsson <daniel@yesql.se> — 2022-08-16T20:34:17Z

> On 16 Aug 2022, at 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I agree though that while simplifying list_length() calls, I'd lean to using
> explicit comparisons to NIL.


Agreed, I prefer that too.

--
Daniel Gustafsson		https://vmware.com/

Re: Propose a new function - list_is_empty

Peter Smith <smithpb2250@gmail.com> — 2022-08-17T01:09:44Z

On Wed, Aug 17, 2022 at 6:34 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 16 Aug 2022, at 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > I agree though that while simplifying list_length() calls, I'd lean to using
> > explicit comparisons to NIL.
>
>
> Agreed, I prefer that too.
>

Thanks for the feedback.

PSA patch v3 which now uses explicit comparisons to NIL everywhere,
and also addresses the other review comments.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Propose a new function - list_is_empty

Daniel Gustafsson <daniel@yesql.se> — 2022-08-17T07:33:24Z

> On 17 Aug 2022, at 03:09, Peter Smith <smithpb2250@gmail.com> wrote:
> 
> On Wed, Aug 17, 2022 at 6:34 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 
>>> On 16 Aug 2022, at 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 
>>> I agree though that while simplifying list_length() calls, I'd lean to using
>>> explicit comparisons to NIL.
>> 
>> 
>> Agreed, I prefer that too.
>> 
> 
> Thanks for the feedback.
> 
> PSA patch v3 which now uses explicit comparisons to NIL everywhere,
> and also addresses the other review comments.

From reading, this version of the patch looks good to me.

--
Daniel Gustafsson		https://vmware.com/

Re: Propose a new function - list_is_empty

Junwang Zhao <zhjwpku@gmail.com> — 2022-08-17T08:13:45Z

There are some places that add extra parenthesis like here

--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -3097,7 +3097,7 @@ reorder_grouping_sets(List *groupingsets, List
*sortclause)
  GroupingSetData *gs = makeNode(GroupingSetData);

  while (list_length(sortclause) > list_length(previous) &&
-    list_length(new_elems) > 0)
+    (new_elems != NIL))
  {

and here,

--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3408,7 +3408,7 @@ estimate_num_groups_incremental(PlannerInfo
*root, List *groupExprs,
  * for normal cases with GROUP BY or DISTINCT, but it is possible for
  * corner cases with set operations.)
  */
- if (groupExprs == NIL || (pgset && list_length(*pgset) < 1))
+ if (groupExprs == NIL || (pgset && (*pgset == NIL)))
  return 1.0;

Is it necessary to add that extra parenthesis?

On Wed, Aug 17, 2022 at 3:33 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 17 Aug 2022, at 03:09, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Wed, Aug 17, 2022 at 6:34 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> >>
> >>> On 16 Aug 2022, at 16:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>
> >>> I agree though that while simplifying list_length() calls, I'd lean to using
> >>> explicit comparisons to NIL.
> >>
> >>
> >> Agreed, I prefer that too.
> >>
> >
> > Thanks for the feedback.
> >
> > PSA patch v3 which now uses explicit comparisons to NIL everywhere,
> > and also addresses the other review comments.
>
> From reading, this version of the patch looks good to me.
>
> --
> Daniel Gustafsson               https://vmware.com/
>
>
>


-- 
Regards
Junwang Zhao

Re: Propose a new function - list_is_empty

Daniel Gustafsson <daniel@yesql.se> — 2022-08-17T08:23:16Z

> On 17 Aug 2022, at 10:13, Junwang Zhao <zhjwpku@gmail.com> wrote:
> 
> There are some places that add extra parenthesis like here
> 
> ...
> 
> Is it necessary to add that extra parenthesis?

It's not necessary unless needed for operator associativity, but also I don't
object to grouping with parenthesis as a visual aid for the person reading the
code.  Going over the patch in more detail I might change my mind on some but I
don't object to the practice in general.

--
Daniel Gustafsson		https://vmware.com/

Re: Propose a new function - list_is_empty

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-17T13:48:19Z

Junwang Zhao <zhjwpku@gmail.com> writes:
> There are some places that add extra parenthesis like here
>   while (list_length(sortclause) > list_length(previous) &&
> -        list_length(new_elems) > 0)
> +        (new_elems != NIL))

> Is it necessary to add that extra parenthesis?

I'd drop the parens in these particular examples because they are
inconsistent with the other parts of the same "if" condition.
I concur with Daniel's point that parens can be useful as a visual
aid even when they aren't strictly necessary --- but I don't think
we should make future readers wonder why one half of the "if"
is parenthesized and the other isn't.

			regards, tom lane

Re: Propose a new function - list_is_empty

Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-17T15:14:12Z

I wrote:
> I'd drop the parens in these particular examples because they are
> inconsistent with the other parts of the same "if" condition.

After going through the patch I removed most but not all of the
newly-added parens on those grounds.  I also found a couple more
spots that could be converted.  Pushed with those changes.

			regards, tom lane

Re: Propose a new function - list_is_empty

Peter Smith <smithpb2250@gmail.com> — 2022-08-17T22:19:47Z

On Thu, Aug 18, 2022 at 1:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > I'd drop the parens in these particular examples because they are
> > inconsistent with the other parts of the same "if" condition.
>
> After going through the patch I removed most but not all of the
> newly-added parens on those grounds.  I also found a couple more
> spots that could be converted.  Pushed with those changes.
>

Thanks for pushing.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.