Remove redundant initializations

On Mon, 28 Jun 2021 at 21:59, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

There are certain parts of code that laboriously initialize every field
of a struct to (some spelling of) zero, even though the whole struct was
just zeroed (by makeNode() or memset()) a few lines earlier. Besides
being redundant, I find this hard to read in some situations because
it's then very hard to tell what is different between different cases or
branches.

Just for information, there was a similar proposal in [1]/messages/by-id/CAFjFpRdmx2oWdCrYcQuk9CZ7S9iTrKSziC==6j0Agw4jdmvLng@mail.gmail.com. There were
some arguments for and against the idea. Might be worth reviewing.

David

[1]: /messages/by-id/CAFjFpRdmx2oWdCrYcQuk9CZ7S9iTrKSziC==6j0Agw4jdmvLng@mail.gmail.com

On 28 Jun 2021, at 11:59, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

There are certain parts of code that laboriously initialize every field of a struct to (some spelling of) zero, even though the whole struct was just zeroed (by makeNode() or memset()) a few lines earlier. Besides being redundant, I find this hard to read in some situations because it's then very hard to tell what is different between different cases or branches. The attached patch cleans up most of that. I left alone instances where there are (nontrivial) comments attached to the initializations or where there appeared to be some value in maintaining symmetry. But a lot of it was just plain useless code, some clearly copy-and-pasted repeatedly.

I personally sort of like the initializations of Lists like the one below, even
if redundant, since they then clearly stand out as being Lists.

- fk_trigger->args = NIL;

Just a matter of personal preference, but I find that those aid readability.

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

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

There are certain parts of code that laboriously initialize every field
of a struct to (some spelling of) zero, even though the whole struct was
just zeroed (by makeNode() or memset()) a few lines earlier.

FWIW, I think that it's an intentional style choice to explicitly
initialize every field rather than relying on makeNode to have done so.

The primary case where I personally rely on that style is when adding a
new field to a struct. Currently it's possible to grep for some existing
field and add the new one beside it. Leaving out initializations by
relying on side-effects of makeNode makes that far riskier.

A different aspect is the one you mention parenthetically, which is
what values can we rely on to be all-zero-bits? Switching to this
style will embed assumptions about that to a far greater degree than
we have now, making the code less robust against changes.

I'm aware that there are opinions to the contrary, but I do not think
this is an improvement.

regards, tom lane

On Tue, 29 Jun 2021 at 02:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary case where I personally rely on that style is when adding a
new field to a struct. Currently it's possible to grep for some existing
field and add the new one beside it. Leaving out initializations by
relying on side-effects of makeNode makes that far riskier.

FWIW, I mostly grep for makeNode(NameOfNode) as I'm a bit mistrusting
of if the random existing field name that I pick to grep for will
properly showing me all the locations I should touch.

David

David Rowley <dgrowleyml@gmail.com> writes:

On Tue, 29 Jun 2021 at 02:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary case where I personally rely on that style is when adding a
new field to a struct. Currently it's possible to grep for some existing
field and add the new one beside it. Leaving out initializations by
relying on side-effects of makeNode makes that far riskier.

FWIW, I mostly grep for makeNode(NameOfNode) as I'm a bit mistrusting
of if the random existing field name that I pick to grep for will
properly showing me all the locations I should touch.

I tend to do that too, but it's not a foolproof thing either, since
some places use random memset's for the purpose.

regards, tom lane

On Mon, Jun 28, 2021 at 3:30 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

There are certain parts of code that laboriously initialize every field
of a struct to (some spelling of) zero, even though the whole struct was
just zeroed (by makeNode() or memset()) a few lines earlier. Besides
being redundant, I find this hard to read in some situations because
it's then very hard to tell what is different between different cases or
branches. The attached patch cleans up most of that. I left alone
instances where there are (nontrivial) comments attached to the
initializations or where there appeared to be some value in maintaining
symmetry. But a lot of it was just plain useless code, some clearly
copy-and-pasted repeatedly.

Note
</messages/by-id/4c9f01be-9245-2148-b569-61a8562ef190@2ndquadrant.com&gt;
where we had a previous discussion about trimming down useless
initializations to zero.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

I'm +1 for the $SUBJECT concept, mostly because I take longer to read code
where immaterial zero-initialization lines are diluting the code. A quick
scan of the patch content is promising. If there's a decision to move
forward, I'm happy to review it more closely.

On Wed, Jun 30, 2021 at 09:28:17AM -0400, Tom Lane wrote:

David Rowley <dgrowleyml@gmail.com> writes:

On Tue, 29 Jun 2021 at 02:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary case where I personally rely on that style is when adding a
new field to a struct. Currently it's possible to grep for some existing
field and add the new one beside it. Leaving out initializations by
relying on side-effects of makeNode makes that far riskier.

FWIW, I mostly grep for makeNode(NameOfNode) as I'm a bit mistrusting
of if the random existing field name that I pick to grep for will
properly showing me all the locations I should touch.

I tend to do that too, but it's not a foolproof thing either, since
some places use random memset's for the purpose.

I checked the first five matches of "git grep ' = T_'" to get a sense of code
sites that skip makeNode(). Just one of those five initializes every field:

recordDependencyOnSingleRelExpr() builds RangeTblEntry, subset of fields
EventTriggerCommonSetup() builds EventTriggerData, full fields
validateForeignKeyConstraint() builds TriggerData, subset of fields
ExecBSInsertTriggers() builds TriggerData, subset of fields [many similar examples in trigger.c]
ExecBuildProjectionInfo() builds ExprState, subset of fields

Hence, I find we're already too inconsistent about "explicitly initialize
every field" to recommend "grep for some existing field". (Two participants
in the 2018 thread made similar observations[1]/messages/by-id/20180830045736.p3mrugcq2j367a3l@alap3.anarazel.de[2]/messages/by-id/CA+TgmoYPw3Y8ZKofseTpVbb8avy7v7JbjmG6BMe7cC+eOd7qVA@mail.gmail.com.) grepping T_NameOfNode
and then makeNode(NameOfNode) is more reliable today, and $SUBJECT will not
decrease its reliability.

[1]: /messages/by-id/20180830045736.p3mrugcq2j367a3l@alap3.anarazel.de
[2]: /messages/by-id/CA+TgmoYPw3Y8ZKofseTpVbb8avy7v7JbjmG6BMe7cC+eOd7qVA@mail.gmail.com

On Sun, Sep 12, 2021 at 06:26:33PM -0700, Noah Misch wrote:

I'm +1 for the $SUBJECT concept, mostly because I take longer to read code
where immaterial zero-initialization lines are diluting the code. A quick
scan of the patch content is promising. If there's a decision to move
forward, I'm happy to review it more closely.

This has been sitting in the CF app for three weeks waiting on author,
so I have marked this entry as RwF for now:
https://commitfest.postgresql.org/34/3229/
--
Michael