Обсуждение: Propose a new function - list_is_empty
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
Вложения
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
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
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.
Вложения
> 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/
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
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
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
> 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/
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
Вложения
> 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/
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
> 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/
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
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
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.