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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
