On Sun, 18 Sept 2022 at 07:59, Peter Geoghegan <p...@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
