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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for the fixes. I really like this way of structuring the warnings more. We might need to polish the text in the messages a bit, and maybe also change the case without definitions, but overall this seems fine. I have a few more comments.


================
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
----------------
piotrdz wrote:
> 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.
I see now. Thanks for explaining!

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:271
@@ +270,3 @@
+void InconsistentDeclarationParameterNameCheck::
+    formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent(
+        const FormatDiagnosticContext &Context) {
----------------
The name is too long for my taste. It doesn't look well with 80-columns limit. Maybe `formatDiagnosticsForDeclarations` or something like this?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:275
@@ +274,3 @@
+    diag(Context.OriginalDeclaration->getLocation(),
+         "function %q0 has %1 other declaration%s1 with differently named "
+         "parameters")
----------------
I'm not a native speaker, but "with differently named parameters" doesn't sound well to me. Maybe "with different parameter names"?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:289
@@ +288,3 @@
+
+    formatDifferingParamsDiagnostic(FormatParamsDiagnosticContext{
+        InconsistentDeclaration.DeclarationLocation, "other declaration",
----------------
I really don't like this idea of introducing a structure for the sole purpose of passing three parameters to a function. I don't see what we gain by doing so, and I would prefer the usual way to pass parameters. The same argument is valid for `formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent` and `formatDiagnosticsInOtherCases`.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:290
@@ +289,3 @@
+    formatDifferingParamsDiagnostic(FormatParamsDiagnosticContext{
+        InconsistentDeclaration.DeclarationLocation, "other declaration",
+        InconsistentDeclaration.DifferingParams});
----------------
Maybe "the other declaration"?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:297
@@ +296,3 @@
+
+void InconsistentDeclarationParameterNameCheck::formatDiagnosticsInOtherCases(
+    const FormatDiagnosticContext &Context, StringRef FunctionDescription,
----------------
The name `formatDiagnosticsInOtherCases` only makes sense when you have the other function name in mind (`formatDiagnosticsInCaseOfOnlyFunctionDeclarationsPresent`). I don't have a good name for it, but even just removing `InOtherCases` would make things better.

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:307
@@ +306,3 @@
+
+    diag(Context.ParameterSourceDeclaration->getLocation(), "%0 seen here",
+         DiagnosticIDs::Level::Note)
----------------
Maybe "the %0 seen here"?

================
Comment at: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp:326
@@ +325,3 @@
+      diag(Context.Location,
+           "differing parameters are named here: (%0), while in %1: (%2)",
+           DiagnosticIDs::Level::Note)
----------------
I'd remove "while".


http://reviews.llvm.org/D12462