Horton

Making C function declaration parameter names consistent with corresponding definition names

Peter Geoghegan <pg@bowt.ie>

View thread View on postgr.es
From: Peter Geoghegan <pg@bowt.ie>
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-09-16T22:42:13Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Harmonize parameter names in pg_dump/pg_dumpall.

  2. Harmonize parameter names in contrib code.

  3. Harmonize more lexer function parameter names.

  4. Harmonize parameter names in ecpg code.

  5. Harmonize lexer adjacent function parameter names.

  6. Harmonize more parameter names in bulk.

  7. Harmonize parameter names in storage and AM code.

  8. Harmonize heapam and tableam parameter names.

  9. Consistently use named parameters in timezone code.

  10. Consistently use named parameters in regex code.

  11. Harmonize reorderbuffer parameter names.

  12. Make check_usermap() parameter names consistent.