Thread: Making C function declaration parameter names consistent with corresponding definition names

I applied clang-tidy's
readability-inconsistent-declaration-parameter-name check with
"readability-inconsistent-declaration-parameter-name.Strict:true" [1]
to write the attached refactoring patch series. The patch series makes
parameter names consistent between each function's definition and
declaration. The check made the whole process of getting everything to
match straightforward.

The total number of lines changed worked out at less than you might
guess it would, since we mostly tend to do this already:

 178 files changed, 593 insertions(+), 582 deletions(-)

I have to admit that these inconsistencies are a pet peeve of mine. I
find them distracting, and have a history of fixing them on an ad-hoc
basis. But there are real practical arguments in favor of being strict
about it as a matter of policy -- it's not *just* neatnikism.

First there is a non-zero potential for bugs by allowing
inconsistencies. Consider the example of the function check_usermap(),
from hba.c. The bool argument named "case_insensitive" is inverted in
the declaration, where it is spelled "case_sensitive". At first I
thought that this might be a security bug, and reported it to
-security as such. It's harmless, but is still arguably something that
might have led to a real bug.

Then there is the "automated refactoring" argument. It would be nice
to make automated refactoring tools work a little better by always (or
almost always) having a clean slate to start with.

In general refactoring work might involve writing a patch that starts
with the declarations that appear in a some .h file of interest in one
pass, and work backwards from there. It might be necessary to switch
dozens of functions over to some new naming convention or parameter
order, so you really want to start with the high level interface in
such a scenario. It's rather nice to be able to use clang-tidy to make
sure that there are no newly introduced inconsistencies -- which have
the potential to be live bugs. It's possible to use clang-tidy for
this process right now, but it's not as easy as it could be because
you have to ignore any preexisting minor inconsistencies. We don't
quite have a clean slate to start from, which makes it more error
prone.

IMV there is a lot to be said for making this a largely mechanical
process, with built in guard rails. Why not lean on the tooling that's
widely available already?

Introducing a project policy around consistent parameter names would
make this easy. I believe that it would be practical and unobtrusive
-- we almost do this already, without any policy in place (it only
took me a few hours to come up with the patch series). I don't think
that we need to create new work for committers to do this.

[1]
https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> I have to admit that these inconsistencies are a pet peeve of mine. I
> find them distracting, and have a history of fixing them on an ad-hoc
> basis. But there are real practical arguments in favor of being strict
> about it as a matter of policy -- it's not *just* neatnikism.

I agree, this has always been a pet peeve of mine as well.  I would
have guessed there were fewer examples than you found, because I've
generally fixed any such cases I happened to notice.

            regards, tom lane

On Fri, Sep 16, 2022 at 4:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I agree, this has always been a pet peeve of mine as well.  I would
> have guessed there were fewer examples than you found, because I've
> generally fixed any such cases I happened to notice.

If you actually go through them all one by one you'll see that the
vast majority of individual cases involve an inconsistency that
follows some kind of recognizable pattern. For example, a Relation
parameter might be spelled "relation" in one place and "rel" in
another. I find these more common cases much less noticeable --
perhaps that's why there are more than you thought there'd be?

It's possible to configure the clang-tidy tooling to tolerate various
inconsistencies, below some kind of threshold -- it is totally
customizable. But I think that a strict, simple rule is the way to go
here. (Though without creating busy work for committers that don't
want to use clang-tidy all the time.)
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> It's possible to configure the clang-tidy tooling to tolerate various
> inconsistencies, below some kind of threshold -- it is totally
> customizable. But I think that a strict, simple rule is the way to go
> here.

Agreed; I see no need to tolerate any inconsistency.

> (Though without creating busy work for committers that don't
> want to use clang-tidy all the time.)

Yeah.  I'd be inclined to handle it about like cpluspluscheck:
provide a script that people can run from time to time, but
don't insist that it's a commit-blocker.  (I wouldn't be unhappy
to see the cfbot include this in its compiler warnings suite,
though, once we get rid of the existing instances.)

            regards, tom lane

On Fri, Sep 16, 2022 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Agreed; I see no need to tolerate any inconsistency.

The check that I used to write the patches doesn't treat unnamed
parameters in a function declaration as an inconsistency, even when
"strict" is used. Another nearby check *could* be used to catch
unnamed parameters [1] if that was deemed useful, though. How do you
feel about unnamed parameters?

Many of the function declarations from reorderbuffer.h will be
affected if we decide that we don't want to allow unnamed parameters
-- it's quite noticeable there. I myself lean towards not allowing
unnamed parameters. (Though perhaps I should reserve judgement until
after I've measured just how prevalent unnamed parameters are.)

> Yeah.  I'd be inclined to handle it about like cpluspluscheck:
> provide a script that people can run from time to time, but
> don't insist that it's a commit-blocker.

My thoughts exactly.

[1] https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-named-parameter.html
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> The check that I used to write the patches doesn't treat unnamed
> parameters in a function declaration as an inconsistency, even when
> "strict" is used. Another nearby check *could* be used to catch
> unnamed parameters [1] if that was deemed useful, though. How do you
> feel about unnamed parameters?

I think they're easily Stroustrup's worst idea ever.  You're basically
throwing away an opportunity for documentation, and that documentation
is often sorely needed.  Handy example:

extern void ReorderBufferCommitChild(ReorderBuffer *, TransactionId, TransactionId,
                                     XLogRecPtr commit_lsn, XLogRecPtr end_lsn);

Which TransactionId parameter is which?  You might be tempted to guess,
if you think you remember how the function works, and that is a recipe
for bugs.

I'd view the current state of reorderbuffer.h as pretty unacceptable on
stylistic grounds no matter which position you take.  Having successive
declarations randomly using named or unnamed parameters is seriously
ugly and distracting, at least to my eye.  We don't need such blatant
reminders of how many cooks have stirred this broth.

            regards, tom lane

On Fri, Sep 16, 2022 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think they're easily Stroustrup's worst idea ever.  You're basically
> throwing away an opportunity for documentation, and that documentation
> is often sorely needed.

He could at least point to C++ pure virtual functions, where omitting
a parameter name in the base class supposedly conveys useful
information. I don't find that argument particularly convincing
myself, even in a C++ context, but at least it's an argument. Doesn't
apply here in any case.

> I'd view the current state of reorderbuffer.h as pretty unacceptable on
> stylistic grounds no matter which position you take.  Having successive
> declarations randomly using named or unnamed parameters is seriously
> ugly and distracting, at least to my eye.  We don't need such blatant
> reminders of how many cooks have stirred this broth.

I'll come up with a revision that deals with that too, then. Shouldn't
be too much more work.

I suppose that I ought to backpatch a fix for the really egregious
issue in hba.h, and leave it at that on stable branches.

-- 
Peter Geoghegan

On Fri, Sep 16, 2022 at 06:48:36PM -0700, Peter Geoghegan wrote:
> On Fri, Sep 16, 2022 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd view the current state of reorderbuffer.h as pretty unacceptable on
>> stylistic grounds no matter which position you take.  Having successive
>> declarations randomly using named or unnamed parameters is seriously
>> ugly and distracting, at least to my eye.  We don't need such blatant
>> reminders of how many cooks have stirred this broth.
>
> I'll come up with a revision that deals with that too, then. Shouldn't
> be too much more work.

Being able to catch unnamed paramaters in function declarations would
be really nice.  Thanks for looking at that.

> I suppose that I ought to backpatch a fix for the really egregious
> issue in hba.h, and leave it at that on stable branches.

If check_usermap() is used in a bugfix, that could be a risk, so this
bit warrants a backpatch in my opinion.
--
Michael

On Fri, Sep 16, 2022 at 6:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Fri, Sep 16, 2022 at 6:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think they're easily Stroustrup's worst idea ever.  You're basically
> > throwing away an opportunity for documentation, and that documentation
> > is often sorely needed.
>
> He could at least point to C++ pure virtual functions, where omitting
> a parameter name in the base class supposedly conveys useful
> information. I don't find that argument particularly convincing
> myself, even in a C++ context, but at least it's an argument. Doesn't
> apply here in any case.

Several files from src/timezone and from src/backend/regex make use of
unnamed parameters in function declarations. It wouldn't be difficult
to fix everything and call it a day, but I wonder if there are any
special considerations here. I don't think that Henry Spencer's regex
code is considered vendored code these days (if it ever was), so that
seems clear cut. I'm less sure about the timezone code.

Note that regcomp.c has a relatively large number of function
declarations that need to be fixed (regexec.c has some too), since the
regex code was written in a style that makes unnamed parameters in
declarations the standard -- we're talking about changing every static
function declaration. The timezone code is just inconsistent about its
use of unnamed parameters, kind of like reorderbuffer.h.

I don't see any reason to treat this quasi-vendored code as special,
but I don't really know anything about your workflow with the timezone
files.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> Several files from src/timezone and from src/backend/regex make use of
> unnamed parameters in function declarations. It wouldn't be difficult
> to fix everything and call it a day, but I wonder if there are any
> special considerations here. I don't think that Henry Spencer's regex
> code is considered vendored code these days (if it ever was), so that
> seems clear cut. I'm less sure about the timezone code.

Yeah, bringing the regex code into line with our standards is fine.
I don't really see a reason not to do it with the timezone code
either, as long as there aren't too many changes there.  We are
carrying a pretty large number of diffs from upstream already.

(Which reminds me that I need to do another update pass on that
code soon.  Not right now, though.)

            regards, tom lane

On Sat, Sep 17, 2022 at 11:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, bringing the regex code into line with our standards is fine.
> I don't really see a reason not to do it with the timezone code
> either, as long as there aren't too many changes there.  We are
> carrying a pretty large number of diffs from upstream already.

I'd be surprised if this created more than 3 minutes of extra work for
you when updating the timezone code.

There are a few places where I had to apply a certain amount of
subjective judgement (rather than just mechanically normalizing the
declarations), but the timezone code wasn't one of those places. Plus
there just isn't that many affected timezone related function
declarations, and they're concentrated in only 3 distinct areas.

-- 
Peter Geoghegan

On Sat, Sep 17, 2022 at 11:36 AM Peter Geoghegan <pg@bowt.ie> wrote:
> I'd be surprised if this created more than 3 minutes of extra work for
> you when updating the timezone code.

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.

BTW. there are just a couple of remaining unfixed
"readability-inconsistent-declaration-parameter-name" warnings --
legitimately tricky cases. These are related to simplehash.h client
code, where we use the C preprocessor to simulate C++ templates
(Bjarne strikes again). It would be possible to suppress the warnings
by making the client code use matching generic function-style
parameter names, but that doesn't seem like an improvement.

If we're going to adapt a project policy around parameter names, then
we'll need a workaround -- probably just by suppressing a handful of
tricky warnings. For now my focus is cleaning things up on HEAD.

-- 
Peter Geoghegan

On Fri, Sep 16, 2022 at 11:59 PM Michael Paquier <michael@paquier.xyz> wrote:
> If check_usermap() is used in a bugfix, that could be a risk, so this
> bit warrants a backpatch in my opinion.

Makes sense. Committed and backpatched a fix for check_usermap() just now

Thanks
-- 
Peter Geoghegan

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

On Sun, Sep 18, 2022 at 4:38 PM David Rowley <dgrowleyml@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

On Sun, Sep 18, 2022 at 5:08 PM Peter Geoghegan <pg@bowt.ie> wrote:
> 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.

On second thought I definitely got this wrong (it's not subjective
after all). I didn't notice that there are actually 2 different
datatypes involved here, justifying a different naming convention for
each. In other words, the problem really was in the .h file, not in
the .c file, so I should simply fix the declaration of
ExecIncrementalSortInitializeWorker() and call it a day.

There is no reason why ExecIncrementalSortInitializeWorker() ought to
be consistent with other functions that appear in the same header
file, since (if you squint) you'll notice that the data types are also
different.

-- 
Peter Geoghegan

On Sun, Sep 18, 2022 at 5:08 PM Peter Geoghegan <pg@bowt.ie> wrote:
> That was left behind following a recent rebase. Will fix.

Attached revision fixes this issue, plus the
ExecIncrementalSortInitializeWorker() issue.

It also adds a lot more fixes which were missed earlier because I
didn't use "strict" when running clang-tidy from the command line
(just in my editor). This includes a fix for the mdexists() issue that
you highlighted.

I expect that this patch series will bitrot frequently, so there is no
good reason to not post revisions frequently.

The general structure of the patchset is now a little more worked out.
Although it's still not close to being commitable, it should give you
a better idea of the kind of structure that I'm aiming for. I think
that this should be broken into a few different parts based on the
area of the codebase affected (not the type of check used). Even that
aspect needs more work, because there is still one massive patch --
this is now the sixth and final patch.

It seems like a good idea to at least have separate commits for both
the regex code and the timezone code, since these are "quasi-vendored"
areas of the code base.

-- 
Peter Geoghegan

On Mon, 19 Sept 2022 at 15:04, Peter Geoghegan <pg@bowt.ie> wrote:
> The general structure of the patchset is now a little more worked out.
> Although it's still not close to being commitable, it should give you
> a better idea of the kind of structure that I'm aiming for. I think
> that this should be broken into a few different parts based on the
> area of the codebase affected (not the type of check used). Even that
> aspect needs more work, because there is still one massive patch --
> this is now the sixth and final patch.

Thanks for updating the patches.

I'm slightly confused about "still not close to being commitable"
along with "this is now the sixth and final patch.".  That seems to
imply that you're not planning to send any more patches but you don't
think this is commitable. I'm assuming I've misunderstood that.

I don't have any problems with 0001, 0002 or 0003.

Looking at 0004 I see a few issues:

1. ConnectDatabase() seems to need a bit more work in the header
comment. There's a reference to AH and AHX.  The parameter is now
called "A".

2. setup_connection() still references AH->use_role in the comments
(line 1102). Similar problem on line 1207 with AH->sync_snapshot_id

3. setupDumpWorker() still makes references to AH->sync_snapshot_id
and AH->use_role in the comments. The parameter is now called "A".

4. dumpSearchPath() still has a comment which references AH->searchpath

0005 looks fine.

I've not looked at 0006 again.

David

On Sun, Sep 18, 2022 at 9:07 PM David Rowley <dgrowleyml@gmail.com> wrote:
> I'm slightly confused about "still not close to being commitable"
> along with "this is now the sixth and final patch.".  That seems to
> imply that you're not planning to send any more patches but you don't
> think this is commitable. I'm assuming I've misunderstood that.

I meant that the "big patch" now has a new order -- it is sixth/last in
the newly revised patchset, v3. I don't know how many more patch
revisions will be required, but at least one or two more revisions
seem likely.

Here is the stuff that it less ready, or at least seems ambiguous:

1. The pg_dump patch is relatively opinionated about how to resolve
inconsistencies, and makes quite a few changes to the .c side.

Seeking out the "lesser inconsistency" resulted in more lines being
changed. Maybe you won't see it the same way (maybe you'll prefer the
other trade-off). That's just how it ended up.

2. The same thing is true to a much smaller degree with the jsonb patch.

3. The big patch itself is...well, very big. And written on autopilot,
to a certain degree. So that one just needs more careful examination,
on general principle.

> Looking at 0004 I see a few issues:
>
> 1. ConnectDatabase() seems to need a bit more work in the header
> comment. There's a reference to AH and AHX.  The parameter is now
> called "A".
>
> 2. setup_connection() still references AH->use_role in the comments
> (line 1102). Similar problem on line 1207 with AH->sync_snapshot_id
>
> 3. setupDumpWorker() still makes references to AH->sync_snapshot_id
> and AH->use_role in the comments. The parameter is now called "A".
>
> 4. dumpSearchPath() still has a comment which references AH->searchpath

Will fix all those in the next revision. Thanks.

--
Peter Geoghegan

On Sun, Sep 18, 2022 at 9:17 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Will fix all those in the next revision. Thanks.

Attached revision v4 fixes those pg_dump patch items.

It also breaks out the ecpg changes into their own patch. Looks like
ecpg requires the same treatment as the timezone code and the regex
code -- it generally doesn't use named parameters in function
declarations, so the majority of its function declarations need to be
adjusted. The overall code churn impact is higher than it was with the
other two modules.

-- 
Peter Geoghegan

On Mon, Sep 19, 2022 at 11:36 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Attached revision v4 fixes those pg_dump patch items.
>
> It also breaks out the ecpg changes into their own patch.

I pushed much of this just now. All that remains to bring the entire
codebase into compliance is the ecpg patch and the pg_dump patch.
Those two areas are relatively tricky. But it's now unlikely that I'll
need to push a commit that makes relatively many CF patches stop
applying against HEAD -- that part is over.

Once we're done with ecpg and pg_dump, we can talk about the actual
practicalities of formally adopting a project policy on consistent
parameter names. I mostly use clang-tidy via my editor's support for
the clangd language server -- clang-tidy is primarily a linter, so it
isn't necessarily run in bulk all that often. I'll need to come up
with instructions for running clang-tidy from the command line that
are easy to follow.

I've found that the run_clang_tidy script (AKA run-clang-tidy.py)
works, but the whole experience feels hobbled together. I think that
we really need something like a build target for this -- something
comparable to what we do to support GCOV. That would also allow us to
use additional clang-tidy checks, which might be useful. We might even
find it useful to come up with some novel check of our own. Apparently
it's not all that difficult to write one from scratch, to implement
custom rules. There are already custom rules for big open source
projects such as the Linux Kernel, Chromium, and LLVM itself.

-- 
Peter Geoghegan

On Tue, Sep 20, 2022 at 1:51 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I pushed much of this just now. All that remains to bring the entire
> codebase into compliance is the ecpg patch and the pg_dump patch.
> Those two areas are relatively tricky. But it's now unlikely that I'll
> need to push a commit that makes relatively many CF patches stop
> applying against HEAD -- that part is over.

Attached revision shows where I'm at with this. Would be nice to get
it all out of the way before too long.

Turns out that we'll need a new patch for contrib, which was missed
before now due to an issue with how I build a compilation database
using bear [1]. The new patch for contrib isn't very different to the
other patches, though. The most notable changes are in pgcrypto and
oid2name. Fairly minor stuff, overall.

[1] https://github.com/rizsotto/Bear
-- 
Peter Geoghegan

On Wed, Sep 21, 2022 at 6:58 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Attached revision shows where I'm at with this. Would be nice to get
> it all out of the way before too long.

Attached is v6, which now consists of only one single patch, which
fixes things up in pg_dump. (This is almost though not quite identical
to the same patch from v5.)

I would like to give another 24 hours for anybody to lodge final
objections to what I've done in this patch. It seems possible that
there will be concerns about how this might affect backpatching, or
something like that. This patch goes relatively far in the direction
of refactoring to make things consistent at the module level -- unlike
most of the patches, which largely consisted of mechanical adjustments
that were obviously correct, both locally and at the whole-module level.

BTW, I notice that meson seems to have built-in support for running
scan-build, a tool that performs static analysis using clang. I'm
pretty sure that it's possible to use scan-build to run clang-tidy
checks (though I've just been using run-clang-tidy myself). Perhaps it
would make sense to use meson's support for scan-build to make it easy
for everybody to run the clang-tidy checks locally.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> I would like to give another 24 hours for anybody to lodge final
> objections to what I've done in this patch. It seems possible that
> there will be concerns about how this might affect backpatching, or
> something like that. This patch goes relatively far in the direction
> of refactoring to make things consistent at the module level -- unlike
> most of the patches, which largely consisted of mechanical adjustments
> that were obviously correct, both locally and at the whole-module level.

Yeah.  I'm not much on board with the AHX->A and AH->A changes you made;
those seem extremely invasive and it's not real clear that they add a
lot of value.

I've never thought that the Archive vs. ArchiveHandle separation in
pg_dump was very well thought out.  I could perhaps get behind a patch
to eliminate that bit of "abstraction"; but I'd still try to avoid
wholesale changes in local-variable names from it.  I don't think that
would buy anything that's worth the back-patching pain.  Just accepting
that Archive[Handle] variables might be named either AH or AHX depending
on historical accident does not seem that bad to me.  We have lots more
and worse naming inconsistencies in our tree.

            regards, tom lane

On Thu, Sep 22, 2022 at 2:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah.  I'm not much on board with the AHX->A and AH->A changes you made;
> those seem extremely invasive and it's not real clear that they add a
> lot of value.

That makes it easy, then. I'll just take the least invasive approach
possible with pg_dump: treat the names from function definitions as
authoritative, and mechanically adjust the function declarations as
needed to make everything agree.

The commit message for this will note in passing that the
inconsistency that this creates at the header file level is a known
issue.

Thanks
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> That makes it easy, then. I'll just take the least invasive approach
> possible with pg_dump: treat the names from function definitions as
> authoritative, and mechanically adjust the function declarations as
> needed to make everything agree.

WFM.

            regards, tom lane

On Thu, Sep 22, 2022 at 3:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> WFM.

Okay, pushed a minimally invasive commit to fix the inconsistencies in
pg_dump related code just now. That's the last of them. Now the only
remaining clang-tidy warnings about inconsistent parameter names are
those that seem practically impossible to fix (these are mostly just
cases involving flex/bison).

It still seems like a good idea to formally create a new coding
standard around C function parameter names. We really need a simple
clang-tidy workflow to be able to do that. I'll try to get to that
soon. Part of the difficulty there will be finding a way to ignore the
warnings that we really can't do anything about.

Thanks
-- 
Peter Geoghegan