Re: Making C function declaration parameter names consistent with corresponding definition names
On Thu, Sep 22, 2022 at 3:20 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Thu, Sep 22, 2022 at 2:55 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Wed, Sep 21, 2022 at 6:58 PM Peter Geoghegan 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
From ae97a00cb4c69b3fd247bc6606509165912bee29 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan
Date: Wed, 21 Sep 2022 14:51:29 -0700
Subject: [PATCH v6] Harmonize parameter names in pg_dump/pg_dumpall.
Make sure that function declarations use names that exactly match the
corresponding names from function definitions. Having parameter names
that are reliably consistent in this way will make it easier to reason
about groups of related C functions from the same translation unit as a
module. It will also make certain refactoring tasks easier.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy.
Author: Peter Geoghegan
Reviewed-By: David Rowley
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
---
src/bin/pg_dump/common.c | 2 +-
src/bin/pg_dump/parallel.c| 14 +-
src/bin/pg_dump/pg_backup.h | 36 ++--
src/bin/pg_dump/pg_backup_archiver.c | 74 +++
src/bin/pg_dump/pg_backup_archiver.h | 10 +-
src/bin/pg_dump/pg_backup_custom.c| 2 +-
src/bin/pg_dump/pg_backup_db.c| 40 ++--
src/bin/pg_dump/pg_backup_db.h| 12 +-
src/bin/pg_dump/pg_backup_directory.c | 2 +-
src/bin/pg_dump/pg_backup_null.c | 8 +-
src/bin/pg_dump/pg_backup_tar.c | 4 +-
src/bin/pg_dump/pg_dump.c | 290 +-
src/bin/pg_dump/pg_dump.h | 6 +-
src/bin/pg_dump/pg_dumpall.c | 6 +-
14 files changed, 254 insertions(+), 252 deletions(-)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 395f817fa..44fa52cc5 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -79,7 +79,7 @@ typedef struct _catalogIdMapEntry
static catalogid_hash *catalogIdHash = NULL;
-static void flagInhTables(Archive *fout, TableInfo *tbinfo, int numTables,
+static void flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
InhInfo *inhinfo, int numInherits);
static void flagInhIndexes(Archive *fout, TableInfo *tblinfo, int numTables);
static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index c8a70d9bc..ba6df9e0c 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -146,7 +146,7 @@ static int pgpipe(int handles[2]);
typedef struct ShutdownInformation
{
ParallelState *pstate;
- Archive*AHX;
+ Archive*A;
} ShutdownInformation;
static ShutdownInformation shutdown_info;
@@ -325,9 +325,9 @@ getThreadLocalPQExpBuffer(void)
* as soon as they've created the ArchiveHandle.
*/
void
-on_exit_close_archive(Archive *AHX)
+on_exit_close_archive(Archive *A)
{
- shutdown_info.AHX = AHX;
+ shutdown_info.A = A;
on_exit_nicely(archive_close_connection, &shutdown_info);
}
@@ -353,8 +353,8 @@ archive_close_connection(int code, void *arg)
*/
ShutdownWorkersHard(si->pstate);
- if (si->AHX)
-DisconnectDatabase(si->AHX);
+ if (si->A)
+DisconnectDatabase(si->A);
}
else
{
@@ -378,8 +378,8 @@ archive_close_connection(int code, void *arg)
else
{
/* Non-parallel operation: just kill the leader DB connection */
- if (si->AHX)
- DisconnectDatabase(si->AHX);
+ if (si->A)
+ DisconnectDatabase(si->A);
}
}
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index fcc5f6bd0..9dc441902 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -270,33 +270,33 @@ typedef int DumpId;
* Function pointer prototypes for assorted callback methods.
*/
-typedef int (*DataDumperPtr) (Archive *AH, const void *userArg);
+typedef int (*DataDumperPtr) (Arc
Re: Making C function declaration parameter names consistent with corresponding definition names
On Mon, Sep 19, 2022 at 11:36 PM Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 9:07 PM David Rowley 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Mon, 19 Sept 2022 at 15:04, Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 5:08 PM Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, Sep 18, 2022 at 4:38 PM David Rowley 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sun, 18 Sept 2022 at 07:59, Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 11:59 PM Michael Paquier 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Sat, Sep 17, 2022 at 11:26 AM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 6:48 PM Peter Geoghegan wrote: > On Fri, Sep 16, 2022 at 6:20 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 06:48:36PM -0700, Peter Geoghegan wrote: > On Fri, Sep 16, 2022 at 6:20 PM Tom Lane 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 signature.asc Description: PGP signature
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 6:20 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 4:49 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
Re: Making C function declaration parameter names consistent with corresponding definition names
On Fri, Sep 16, 2022 at 4:19 PM Tom Lane 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
Re: Making C function declaration parameter names consistent with corresponding definition names
Peter Geoghegan 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
