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

[Piotr Dziwinski via cfe-commits]

piotrdz marked 7 inline comments as done.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:23
@@ +22,3 @@
+
+struct CheckResult {
+  CheckResult(bool HasInconsistentParams,
----------------
alexfh wrote:
> 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?
In the end, I used SourceLocation.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:33
@@ +32,3 @@
+
+struct InconsistentParamChecker {
+  static CheckResult checkForInconsitentDeclarationParameters(const FunctionDecl* FunctionDeclaration);
----------------
alexfh wrote:
> Why do you need a struct? It's not Java, free functions are totally fine.
That was my sort of "ingenious" workaround for not being able to do this:
```
namespace {
void checkForInconsitentDeclarationParameters(MatchFinder *Finder); // declaration
}
void checkForInconsitentDeclarationParameters(MatchFinder *Finder) { /*..*/ } // definition
```

But I suppose we're better off without such inventions ;). So I moved it all into one anonymous namespace.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:53
@@ +52,3 @@
+
+  auto CheckResult = InconsistentParamChecker::checkForInconsitentDeclarationParameters(FunctionDeclaration);
+  if (CheckResult.HasInconsistentParams) {
----------------
alexfh wrote:
> 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.
All right, I didn't know about such rule here. In my colobot-lint code I prefer the use of auto whenever possible, to isolate my code from possible API changes. But here, as part of Clang project itself, I guess it's not necessary.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:59
@@ +58,3 @@
+
+    diag(FunctionDeclaration->getLocation(), "function '%0' has other declaration with different parameter name(s)")
+      << QualName;
----------------
alexfh wrote:
> 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.
All right, I added note messages about parameter names.

As to adding FixIt hints, I also think they're more useful than simple diagnostic, but in my opinion, it can be difficult to tell what is the intended name in given case. I think there are two cases we can consider:
 - If we see the definition of function in current translation unit, we may assume that the most up-to-date parameter names are those of the definition, since they are the closest to actual code that uses them in some context. So the proposed fix would be to update all other declarations we see to match the definition.
 - If we see only declarations in current translation unit, things start getting difficult, as we (usually) have no way to tell which declaration is considered more important than any other. In such case, I would not propose any hints and leave it up to the user to decide what to do.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h:64
@@ +63,3 @@
+private:
+    std::set<std::string> ReportedFunctions;
+};
----------------
alexfh wrote:
> If you don't need these to be sorted, you can use `llvm::StringSet<>` which should be more effective.
I went with llvm::DenseSet<>, as suggested below

================
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
+
----------------
alexfh wrote:
> Please add tests for special functions (constructor, copy operator), implicit functions, template functions with several instantiations, function declarations in macros.
Thanks for pointing it out. I found there are some problems with formatting note messages with templates, but I don't know how to solve them yet :-(


http://reviews.llvm.org/D12462