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

[Alexander Kornienko via cfe-commits]

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