On Sun, Sep 18, 2022 at 4:38 PM David Rowley <dgrowle...@gmail.com> wrote: > 1. In getJsonPathVariable you seem to have mistakenly removed a > parameter from the declaration.
That was left behind following a recent rebase. Will fix. Every other issue you've raised is some variant of: "I see that you've made a subjective decision to resolve this particular inconsistency on the declaration side by following this particular approach. Why did you do it that way?" This is perfectly reasonable, and it's possible that I made clear mistakes in some individual cases. But overall it's not surprising that somebody else wouldn't handle it in exactly the same way. There is no question that some of these decisions are a little arbitrary. > 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()? Because it somehow felt slightly preferable than introducing a .h level inconsistency between ScanECPGKeywordLookup() and ScanCKeywordLookup(). This is about as hard to justify as justifying why one prefers a slightly different shade of beige when comparing two pages from a book of wallpaper samples. > 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. Again, greater consistency at the .h level won out here. Granted it's still not perfectly consistent, since I didn't take that to its logical conclusion and make sure that the .h file was consistent, because then we'd be talking about why I did that. :-) > 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.) Again, I was looking at this at the level of the .h file (in this case nodeIncrementalSort.h). It never occurred to me to consider other *InitializeWorker() functions. Offhand I think that we should change all of the other *InitializeWorker() functions. I think that things got like this because somebody randomly made one of them pwcxt at some point, which was copied later on. > 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. Yeah, I think that we might as well be perfectly consistent. Making automated refactoring tools work better here is of course a goal of mine -- which is especially useful for making everything consistent at the whole-interface (or header file) level. I wasn't sure how much of that to do up front vs in a later commit. -- Peter Geoghegan
