On Sun, 18 Sept 2022 at 07:59, Peter Geoghegan <pg@bowt.ie> wrote:
> Attached revision adds a new, third patch. This fixes all the warnings
> from clang-tidy's "readability-named-parameter" check. The extent of
> the code churn seems acceptable to me.
+1 to the idea of aligning the parameter names between function
declarations and definitions.
I had a look at the v2-0001 patch and noted down a few things while reading:
1. In getJsonPathVariable you seem to have mistakenly removed a
parameter from the declaration.
2. You changed the name of the parameter in the definition of
ScanCKeywordLookup(). Is it not better to keep the existing name there
so that that function is consistent with ScanKeywordLookup()?
3. Why did you rename the parameter in the definition of
nocachegetattr()? Wouldn't it be better just to rename in the
declaration. To me, "tup" does not really seem better than "tuple"
here.
4. In the definition of ExecIncrementalSortInitializeWorker() you've
renamed pwcxt to pcxt, but it seems that the other *InitializeWorker()
functions call this pwcxt. Is it better to keep those consistent? I
understand that you've done this for consistency with *InitializeDSM()
and *Estimate() functions, but I'd rather see it remain consistent
with the other *InitializeWorker() functions instead. (I'd not be
against a wider rename so all those functions use the same name.)
5. In md.c I see you've renamed a few "forkNum" variables to
"formnum". Maybe it's worth also doing the same in mdexists().
mdcreate() is also external and got the rename, so I'm not quite sure
why mdexists() would be left.
David