[PATCH] D12462: [PATCH] [clang-tidy] Add inconsistent declaration parameter name check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

> Now that I fixed all review issues, I think this version would be acceptable for commit. @alexfh: do you agree?


Almost. I've found a few more minor issues and also found out that the warnings could structured in a more useful way. See the inline comments.

Thanks for working on this!


================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:60
@@ +59,3 @@
+  bool IsTemplateSpecialization;
+  DifferingParamsContainer DifferingParams;
+};
----------------
That's what I would do as well: the parameter names from the function definition can be the source of truth (maybe only when they are actually _used_ inside the function body, because unused arguments don't tell us much), everything else is more likely to be outdated.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:106
@@ +105,3 @@
+      if (IsTemplateSpecialization) {
+        // Template specializations need special handling.
+        // What we actually see here is a generated declaration from the main
----------------
I don't see why we couldn't just ignore compiler-generated template specializations (hand-written ones should probably be handled). It should be easy to filter them out by adding `unless(isInTemplateInstantiation())` inside the `functionDecl` matcher.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:141
@@ +140,3 @@
+
+  return FunctionDeclaration; // No definition found, so return original
+                              // declaration.
----------------
nit: I'd put the comment right before the `return`.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:147
@@ +146,3 @@
+    const DifferingParamsContainer &DifferingParams,
+    std::function<StringRef(const DifferingParamInfo &)> ParamNameChooser) {
+  std::ostringstream Str;
----------------
nit: I think, callable variables should be named more like functions, so `ChooseParamName` would be better.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:148
@@ +147,3 @@
+    std::function<StringRef(const DifferingParamInfo &)> ParamNameChooser) {
+  std::ostringstream Str;
+  bool First = true;
----------------
It's preferred to use `SmallVector<>` and `raw_svector_ostream` in LLVM code to avoid extra heap allocations.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:172
@@ +171,3 @@
+    const MatchFinder::MatchResult &Result) {
+  const FunctionDecl *FunctionDeclaration =
+      Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
----------------
nit: It's better to use `const auto *` to avoid repeating the type name.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:174
@@ +173,3 @@
+      Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
+  if (FunctionDeclaration == nullptr)
+    return;
----------------
This should never happen, since the type of the getNodeAs template argument is the same as the type of the matched node with this identifier, and there's no optional branches in the matcher. This check is redundant.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:188
@@ +187,3 @@
+    markRedeclarationsAsVisited(
+        FunctionDeclaration); // Avoid unnecessary further visits.
+    return;
----------------
nit: If you put the comment above the statement, you'll avoid this weird line break.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:196
@@ +195,3 @@
+      << FunctionDeclaration
+      << static_cast<int>(InconsistentDeclarations.size());
+
----------------
Why is the cast needed here?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:213
@@ +212,3 @@
+                              [](const DifferingParamInfo &ParamInfo)
+                                  -> StringRef { return ParamInfo.OtherName; })
+        << joinParameterNames(InconsistentDeclaration.DifferingParams,
----------------
Is there a specific reason to specify the return type here (`-> StringRef`)?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:227
@@ +226,3 @@
+    }
+    // TODO: Decide what to do if we see only declarations and no definition.
+
----------------
I don't think we can choose any of the declarations as the source of truth. Choosing more frequently used parameter names could work, if there are more than two declarations and just one of them has different names, but this seems to be a corner case rather than a regular practice to have more than two declarations.

This TODO seems to be the best solution for now.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h:1
@@ +1,2 @@
+//===--- InconsistentDeclarationParameterNameCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
----------------
nit: Please fit the line to 80 characters by removing extra minuses.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h:21
@@ +20,3 @@
+
+/// \brief Checks for declarations of functions which differ in parameter names
+///
----------------
nit: Please add trailing period.

================
Comment at: test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:21
@@ +20,3 @@
+
+// CHECK-MESSAGES: :[[@LINE+5]]:6: warning: function 'inconsistentFunctionWithVisibleDefinition' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name]
+// CHECK-MESSAGES: :[[@LINE+3]]:6: note: 1st inconsistent declaration seen here
----------------
I've just noticed that the warning appears on the function definition in this case. It would be more logical to issue a separate warning per each declaration where we think the parameter names are wrong. It's easier for the user to understand when the warning shown is where the (likely) incorrect code is, and when the fix is local to the warning (and not applied somewhere in a different file).

It also works better with features like `-line-filter` and the clang-tidy-diff.py script.

Here's how it would look like:

```
a.h:
...
10| void f(int x);
...

b.h:
...
20| void f(int y);
...

file.cpp:
...
30| void f(int z) {}
...

// a.h:10:6: warning: function 'f' has a definition with different parameter name(s) ('z')
// the fix for a.h is attached to this warning
// file.cpp:30:6: note: function 'f' defined here

// b.h:20:6: warning: function 'f' has a definition with different parameter name(s) ('z')
// the fix for b.h is attached to this warning
// file.cpp:30:6: note: function 'f' defined here
```

================
Comment at: test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:43
@@ +42,3 @@
+struct SpecialFunctions {
+// CHECK-MESSAGES: :[[@LINE+13]]:19: warning:  function 'SpecialFunctions::SpecialFunctions' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name]
+// CHECK-MESSAGES: :[[@LINE+3]]:3: note: 1st inconsistent declaration seen here
----------------
nit: To make the test more readable, it's better to truncate warning messages in CHECK-MESSAGES lines leaving all dynamic information and a bit of text to make the message recognizable. The whole message should be present once in the test file.


http://reviews.llvm.org/D12462