Обсуждение: Making C function declaration parameter names consistent with corresponding definition names
Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Michael Paquier
Дата:
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
Вложения
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Вложения
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Вложения
- v3-0001-Harmonize-parameter-names-in-regex-code.patch
- v3-0004-Harmonize-parameter-names-in-pg_dump-pg_dumpall.patch
- v3-0005-Harmonize-parameter-names-in-jsonb-code.patch
- v3-0006-Harmonize-parameter-names-in-miscellaneous-code.patch
- v3-0003-Harmonize-parameter-names-in-heapam-and-related-c.patch
- v3-0002-Harmonize-parameter-names-in-timezone-code.patch
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Вложения
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Вложения
Re: Making C function declaration parameter names consistent with corresponding definition names
От
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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
Re: Making C function declaration parameter names consistent with corresponding definition names
От
Peter Geoghegan
Дата:
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