alexfh added a comment.
Thanks for contributing the new check!
This check seems pretty similar to the check implemented in
clang-tidy/readability/NamedParameterCheck.h/.cpp. Having both doesn't make
much sense, so one of them will have to die (probably, the other one). Nothing
should be done to this effect now, we'll figure out after this check is
polished enough.
See a few other comments inline.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:23
@@ +22,3 @@
+
+struct CheckResult {
+ CheckResult(bool HasInconsistentParams,
----------------
Maybe use `llvm::Optional<SourceLocation>`? Or, if you don't need to store
invalid locations, just use `SourceLocation` and use `SourceLocation()` as a
magic value?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:33
@@ +32,3 @@
+
+struct InconsistentParamChecker {
+ static CheckResult checkForInconsitentDeclarationParameters(const
FunctionDecl* FunctionDeclaration);
----------------
Why do you need a struct? It's not Java, free functions are totally fine.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:43
@@ +42,3 @@
+ unless(anyOf(
+ isExpansionInSystemHeader(),
+ isImplicit())))
----------------
Clang-tidy takes care of filtering out non-user code, so there's no need to do
this in the check. There's even a special mode for running clang-tidy on system
libraries (for library developers), which this condition will interfere with.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:53
@@ +52,3 @@
+
+ auto CheckResult =
InconsistentParamChecker::checkForInconsitentDeclarationParameters(FunctionDeclaration);
+ if (CheckResult.HasInconsistentParams) {
----------------
We use `auto` when the type is obvious from the RHS (e.g. line 49 above) or
when the specific type is complex and not particularly helpful (e.g. when
iterating over std::map<>, or for iterators in general). In almost all other
cases the specific type is preferred.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:54
@@ +53,3 @@
+ auto CheckResult =
InconsistentParamChecker::checkForInconsitentDeclarationParameters(FunctionDeclaration);
+ if (CheckResult.HasInconsistentParams) {
+ auto QualName = FunctionDeclaration->getQualifiedNameAsString();
----------------
nit: I'd prefer to use early return here:
if (!CheckResult.HasInconsistentParams)
return;
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:56
@@ +55,3 @@
+ auto QualName = FunctionDeclaration->getQualifiedNameAsString();
+ if (ReportedFunctions.count(QualName) > 0)
+ return; // avoid multiple warnings
----------------
This could be stored more effectively as a set of pointers to canonical
declarations (llvm::DenseSet<FunctionDecl*>).
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:59
@@ +58,3 @@
+
+ diag(FunctionDeclaration->getLocation(), "function '%0' has other
declaration with different parameter name(s)")
+ << QualName;
----------------
The message could be more helpful, if it contained the differing names of the
parameters.
A significant improvement would be to propose a fix when it's more or less
clear what the correct name of each parameter should be (e.g. 2 of 3
declarations have the same parameter names, and the third one has different
names, or we could consider parameter names used in the definition correct).
Without a fix, the value of the check will be much lower. In my experience,
developers are reluctant to fix readability warnings manually.
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:81
@@ +80,3 @@
+
+ if (!MyParamName.empty() &&
+ !OtherParamName.empty() &&
----------------
Code formatting is sub-optimal here. Could you clang-format the code?
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h:22
@@ +21,3 @@
+
+/// \brief Checks for declarations of functions which differ in parameter names
+///
----------------
It seems that the documentation here is different from the one in the .rst
file. I'd remove the documentation from here (after syncing the recent changes
to .rst, if needed) and just leave a one-sentence summary and the URL where the
.rst document is going to be published
(http://clang.llvm.org/extra/clang-tidy/checks/<check-name>.html).
================
Comment at:
clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h:64
@@ +63,3 @@
+private:
+ std::set<std::string> ReportedFunctions;
+};
----------------
If you don't need these to be sorted, you can use `llvm::StringSet<>` which
should be more effective.
================
Comment at:
test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:1
@@ +1,2 @@
+// RUN: %python %S/check_clang_tidy.py %s
readability-inconsistent-declaration-parameter-name %t
+
----------------
Please add tests for special functions (constructor, copy operator), implicit
functions, template functions with several instantiations, function
declarations in macros.
http://reviews.llvm.org/D12462
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
