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

[Piotr Dziwinski via cfe-commits]

piotrdz added a comment.

@alexfh: What do you think now? Are we getting nearer to making a commit?


================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:60
@@ +59,3 @@
+  bool IsTemplateSpecialization;
+  DifferingParamsContainer DifferingParams;
+};
----------------
alexfh wrote:
> 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.
All right. I added a check for whether the parameter is referenced.

================
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
----------------
alexfh wrote:
> 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.
Unfortunately, it doesn't work like that. We get this declaration not from matcher, but by iterating redecls().

In any case, I reconsidered this problem, and I came to conclusion that since we need special code for handling template specializations, we may as well do it properly. The only reason that this version of code works at all, is because of incidental generation of this special declaration appearing in the same place where we have function specialization. This, I think, should be considered a side effect of how AST generation works now, and not how it must necessarily work. It may change or even disappear in the future, making this code brittle.

To do this correctly, I believe we should retrieve main template declaration by using `getPrimaryTemplate()->getTemplatedDecl()` and process that. This fixes the issue of wrong location reporting, and also makes it clear what is happening to someone reading the code.

So in the end, partly because of this issue, I ended up rewriting over half of my code, but what we're left with is I think much better.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:196
@@ +195,3 @@
+      << FunctionDeclaration
+      << static_cast<int>(InconsistentDeclarations.size());
+
----------------
alexfh wrote:
> Why is the cast needed here?
Because without it, there is comple error about ambiguous operator<< overload. SmallVector::size() returns unsigned long, while DiagnosticBuilder provides operator<< overloads only for int and unsigned int.

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

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:227
@@ +226,3 @@
+    }
+    // TODO: Decide what to do if we see only declarations and no definition.
+
----------------
alexfh wrote:
> 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.
All right. In new version of code, I moved this comment to extracted function checkIfFixItHintIsApplicable() and extended it to document the current reasoning.

================
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
----------------
alexfh wrote:
> 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
> ```
I didn't think of it this way, but you're right, the user would expect the diagnostics to be at the declaration location. I changed the code to reflect that.

================
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
----------------
alexfh wrote:
> 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.
OK. I didn't know I could do that, thanks.


http://reviews.llvm.org/D12462