Thread: pgsql: Harmonize reorderbuffer parameter names.
Harmonize reorderbuffer parameter names. Make reorderbuffer.h function declarations consistently use named parameters. Also make sure that the declarations use names that match corresponding names from function definitions in reorderbuffer.c. This makes the definitions easier to follow, especially in the case of functions that happen to have adjoining arguments of the same type. This patch was written with help from clang-tidy. Specifically, its "readability-inconsistent-declaration-parameter-name" check and its "readability-named-parameter" check were used. Author: Peter Geoghegan <pg@bowt.ie> Discussion: https://postgr.es/m/3955318.1663377656@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/035ce1feb2ed27680558ebc5cd1455041c8ec3cf Modified Files -------------- src/backend/replication/logical/reorderbuffer.c | 6 +-- src/include/replication/reorderbuffer.h | 72 ++++++++++++++----------- 2 files changed, 43 insertions(+), 35 deletions(-)
Peter Geoghegan <pg@bowt.ie> writes:
> Harmonize reorderbuffer parameter names.
I'm confused about this, because it didn't fix the (ahem) poster-child
case?
-extern void ReorderBufferAssignChild(ReorderBuffer *, TransactionId, TransactionId, XLogRecPtr commit_lsn);
-extern void ReorderBufferCommitChild(ReorderBuffer *, TransactionId, TransactionId,
+extern void ReorderBufferAssignChild(ReorderBuffer *rb, TransactionId xid,
+ TransactionId subxid, XLogRecPtr lsn);
+extern void ReorderBufferCommitChild(ReorderBuffer *rb, TransactionId, TransactionId,
XLogRecPtr commit_lsn, XLogRecPtr end_lsn);
ReorderBufferAssignChild is better, but ReorderBufferCommitChild is still
short some parameter names.
regards, tom lane
On Sat, Sep 17, 2022 at 9:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > ReorderBufferAssignChild is better, but ReorderBufferCommitChild is still > short some parameter names. I actually noticed that clang-tidy missed this one myself, shortly after commit. I still haven't figured out why, but it seems like it might be a bug in the readability-named-parameter check. Could also have something to do with my clang tooling config, even though it works well overall. I'll fix this soon, in any case. -- Peter Geoghegan
On Sat, Sep 17, 2022 at 10:00 PM Peter Geoghegan <pg@bowt.ie> wrote: > I actually noticed that clang-tidy missed this one myself, shortly > after commit. I still haven't figured out why, but it seems like it > might be a bug in the readability-named-parameter check. Could also > have something to do with my clang tooling config, even though it > works well overall. I see what happened here: while the "readability-inconsistent-declaration-parameter-name" check works equally well with extern functions and functions with local linkage, the same cannot be said for the "readability-named-parameter" check. The latter only seems to work with functions that have local linkage. I suppose that this limitation might have something to do with how clang-tidy reasons about function declarations, a process that is bound to be constrained by how extern declarations work in general. A C compiler never compiles a .h file; it compiles translation units. In short, it is probably a bad idea for the tool to attribute problems to a symbol from an included .h file because that interpretation would have to be predicated on how the file was included -- an interpretation which might have it backwards. In general clang-tidy is only willing to complain about function declarations for static functions. My fixes for inconsistencies that were reported as function definition inconsistencies usually addressed the problem by adjusting the .h side. Strictly speaking, that approach relies on an understanding that affected extern declarations from .h files are canonical. This is an understanding that clang-tidy cannot rely on. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> I see what happened here: while the
> "readability-inconsistent-declaration-parameter-name" check works
> equally well with extern functions and functions with local linkage,
> the same cannot be said for the "readability-named-parameter" check.
> The latter only seems to work with functions that have local linkage.
That's kind of annoying --- seems to put a serious crimp in any plans
to check this mechanically.
regards, tom lane
On Sun, Sep 18, 2022 at 2:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > That's kind of annoying --- seems to put a serious crimp in any plans > to check this mechanically. I don't see why it should make a huge difference. Granted we can't really rely on the "readability-named-parameter" check in the way we'd hoped, but AFAICT we can rely on the "readability-inconsistent-declaration-parameter-name" check to work everywhere. The latter check is far more important in practice, I think, because people don't tend to omit parameter names very often. One further caveat here is that I seem to need to set "IgnoreMacros: false" to get perfect results for the "inconsistent" check when the C preprocessor is involved, as it often is (e.g., with TransactionId params). Even if some other limitations become apparent, we can probably afford to allow some false negatives. I don't see any evidence of that so far, barring this issue with unnamed parameter checking. -- Peter Geoghegan
On Sun, Sep 18, 2022 at 2:14 PM Peter Geoghegan <pg@bowt.ie> wrote: > One further caveat here is that I seem to need to set "IgnoreMacros: > false" to get perfect results for the "inconsistent" check when the C > preprocessor is involved, as it often is (e.g., with TransactionId > params). Actually that explanation is wrong. In any case trial and error has shown that "IgnoreMacros=false" [1] and "Strict=true" produce the most useful results. [1] https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html#cmdoption-arg-IgnoreMacros -- Peter Geoghegan