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