Thread: Remove redundant initializations
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 <https://www.postgresql.org/message-id/flat/4c9f01be-9245-2148-b569-61a8562ef190@2ndquadrant.com> where we had a previous discussion about trimming down useless initializations to zero.
Attachment
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]. There were some arguments for and against the idea. Might be worth reviewing. David [1] https://www.postgresql.org/message-id/flat/CAFjFpRdmx2oWdCrYcQuk9CZ7S9iTrKSziC%3D%3D6j0Agw4jdmvLng%40mail.gmail.com#ff36253217d67d5531f5b2017a0dbfd0
> 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 thoughthe whole struct was just zeroed (by makeNode() or memset()) a few lines earlier. Besides being redundant, I findthis hard to read in some situations because it's then very hard to tell what is different between different cases orbranches. The attached patch cleans up most of that. I left alone instances where there are (nontrivial) comments attachedto the initializations or where there appeared to be some value in maintaining symmetry. But a lot of it was justplain 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 > <https://www.postgresql.org/message-id/flat/4c9f01be-9245-2148-b569-61a8562ef190@2ndquadrant.com> > 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][2].) grepping T_NameOfNode and then makeNode(NameOfNode) is more reliable today, and $SUBJECT will not decrease its reliability. [1] https://postgr.es/m/20180830045736.p3mrugcq2j367a3l@alap3.anarazel.de [2] https://postgr.es/m/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