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

[piotrdz]

Summary

This is first of series of patches, porting code from my project colobot-lint, as I mentioned recently in cfe-dev mailing list.

This patch adds a new check in readability module: readability-inconsistent-declaration-parameter-name. I also added appropriate testcases and documentation.

I chose readability module, as it seems it is the best place for it.

I think I followed the rules of LLVM coding guideline, but I may have missed something, as I usually use other code formatting style.

[piotrdz]

I noticed a few things I should have done better:

• use std::set instead of std::unordered_set (it doesn't seem to be used elsewhere)
• move checking set of reported functions to just before emitting diagnostic
• improved documentation
• changed diff directory to inside clang-tools-extra repository

[alexfh]

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.

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

24

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?

34

Why do you need a struct? It's not Java, free functions are totally fine.

44

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.

54

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.

55

nit: I'd prefer to use early return here:

if (!CheckResult.HasInconsistentParams)
  return;

57

This could be stored more effectively as a set of pointers to canonical declarations (llvm::DenseSet<FunctionDecl*>).

60

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.

82

Code formatting is sub-optimal here. Could you clang-format the code?

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

23

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).

65

If you don't need these to be sorted, you can use llvm::StringSet<> which should be more effective.

test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp

2

Please add tests for special functions (constructor, copy operator), implicit functions, template functions with several instantiations, function declarations in macros.

[piotrdz]

@alexfh: Thanks for reviewing my code. The amount of comments makes it pretty clear that it's my first contribution here :-). But no worries, I'll address each issue.

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.

Ah, I only now noticed it. It's kind of "opposite" to my check - it checks for unnamed parameters that should have their names in comments, while my check completely ignores unnamed parameters and checks the named ones. Anyways, I agree, that it would be good to merge them and have one check do both things.

[piotrdz]

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

24

In the end, I used SourceLocation.

34

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.

54

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.

60

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.

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

65

I went with llvm::DenseSet<>, as suggested below

test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp

2

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 :-(

[piotrdz]

Applied fixes for most issues found in review.

[piotrdz]

Now this issue with templates is a bit difficult for me. I tried everything that seems to have made sense, but I still get output like this:

/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: warning: function 'templateFunction' has other declaration with different parameter name [readability-inconsistent-declaration-parameter-name]
void templateFunction(float b)
     ^
/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: note: other declaration seen here
void templateFunction(float b)
     ^
/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:29: note: parameter 1 is named 'b' here, but 'a' in other declaration
void templateFunction(float b)
                            ^

But I'd like to get output like this:

/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: warning: function 'templateFunction<float>' has other declaration with different parameter name [readability-inconsistent-declaration-parameter-name]
void templateFunction(float b)
     ^
/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: note: other declaration seen here
void templateFunction(T a);
     ^
/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:29: note: parameter 1 is named 'b' here, but 'a' in other declaration
void templateFunction(float b)
                            ^

[piotrdz]

In this third version I did the following:

• fixed problems which I noticed with template specializations,
• changed output diagnostics to be more detailed, covering multiple declarations
• added FixIt hints to refactor inconsistent declarations to parameter names we see in definition
• added new template-related testcases and extended existing ones with checking of note diagnostics and FixIt hints

[alexfh]

Sorry for the long delay.

This version looks significantly better. Thank you for the updates!

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

2

Seems like clang-format (or something else) broke this comment.

18

nit: If you move this below namespace clang {, you can omit the clang:: part.

39

I think, llvm:: is not needed here, as StringRef should also be declared in namespace clang.

87

Please use proper Capitalization and punctuation. http://llvm.org/docs/CodingStandards.html#commenting

Same in other comments.

181

That can quickly become extremely chatty. Maybe cramp two lists of names in a single message? Something along the lines of: "parameter names here: (a, b, c, d), in the other declaration: (q, u, x)"

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

25

nit: I'd say "For detailed documentation see:"

[piotrdz]

I addressed all latest review issues.

[piotrdz]

Now that I fixed all review issues, I think this version would be acceptable for commit. @alexfh: do you agree?
There are of course two outstanding issues marked with TODO and FIXME comments, but these are areas of improvement. I would like to first discuss the best solution to them, before attempting to fix them in code.

[alexfh]

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!

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

61

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.

107

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.

142

nit: I'd put the comment right before the return.

148

nit: I think, callable variables should be named more like functions, so ChooseParamName would be better.

149

It's preferred to use SmallVector<> and raw_svector_ostream in LLVM code to avoid extra heap allocations.

173

nit: It's better to use const auto * to avoid repeating the type name.

175

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.

189

nit: If you put the comment above the statement, you'll avoid this weird line break.

197

Why is the cast needed here?

214

Is there a specific reason to specify the return type here (-> StringRef)?

228

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.

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

2

nit: Please fit the line to 80 characters by removing extra minuses.

22

nit: Please add trailing period.

test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp

22

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

44

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.

[piotrdz]

I hope this is the final re-write of my code. In this version, I addressed most recent review comments, while also refactoring code to better handle template specializations and correctly display diagnostics in case when function definition is visible. In the end I had to change over half of the code, but perhaps it was worth it.

[piotrdz]

@alexfh: What do you think now? Are we getting nearer to making a commit?

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

61

All right. I added a check for whether the parameter is referenced.

107

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.

197

Because without it, there is comple error about ambiguous operator<< overload. SmallVector::size() returns unsigned long, while DiagnosticBuilder provides operator<< overloads only for int and unsigned int.

214

No, I just prefer that style. Anyway, I removed it.

228

All right. In new version of code, I moved this comment to extracted function checkIfFixItHintIsApplicable() and extended it to document the current reasoning.

test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp

22

I didn't think of it this way, but you're right, the user would expect the diagnostics to be at the declaration location. I changed the code to reflect that.

44

OK. I didn't know I could do that, thanks.

[alexfh]

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.

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

107

I see now. Thanks for explaining!

272

The name is too long for my taste. It doesn't look well with 80-columns limit. Maybe formatDiagnosticsForDeclarations or something like this?

276

I'm not a native speaker, but "with differently named parameters" doesn't sound well to me. Maybe "with different parameter names"?

290

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.

291

Maybe "the other declaration"?

298

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.

308

Maybe "the %0 seen here"?

327

I'd remove "while".

[piotrdz]

Again, addressed all review issues. I hope this is the final version

[piotrdz]

@alexfh: here we go again. Any comments?

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

290

There is something to gain by it. Although in the end this proved to be a false problem, and I was able to get rid of these structures, I'll explain my reasoning.

The problem I was trying to solve here by introducing these structures, is actually the problem ofpassing reference to the results in InconsistentDeclarationsContainer and DifferingParamsContainer.

C++ does not allow for forward declarations of typedefs, so I cannot just say class InconsistentDeclarationsContainer; in header file. I have to provide full typedef involving SmallVector and its magic number in second template argument. This, I believe, is unnecessary introduction of implementation detail in header file.

So you see, I created such problem for myself, and this is the solution that I came up with.

However, now that I looked again at this, I saw a simpler solution.

All these functions formatting diagnostics, use only one member function of the check class: diag(). I was working under the assumption that it was a protected member of ClangTidyCheck. But I was wrong. It's public. So these don't have to be member functions.*Poof* and all our problems are gone :-)

[alexfh]

Looks good now.
Thank you for the hard work! I'll commit the patch for you.

[piotrdz]

\o/ Yay! Thanks!

[alexfh]

Committed revision 247261.

Thank you again for the new awesome check!

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
+  InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   ShrinkToFitCheck.cpp
   SimplifyBooleanExprCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

+//===- InconsistentDeclarationParameterNameCheck.h - clang-tidy-*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENT_DECLARATION_PARAMETER_NAME_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENT_DECLARATION_PARAMETER_NAME_H
+
+#include "../ClangTidy.h"
+
+#include "llvm/ADT/DenseSet.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Checks for declarations of functions which differ in parameter names.
+///
+/// For detailed documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html
+///
+class InconsistentDeclarationParameterNameCheck : public ClangTidyCheck {
+public:
+  InconsistentDeclarationParameterNameCheck(StringRef Name,
+                                            ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void markRedeclarationsAsVisited(const FunctionDecl *FunctionDeclaration);
+
+  llvm::DenseSet<const FunctionDecl *> VisitedDeclarations;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENT_DECLARATION_PARAMETER_NAME_H

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

+//===--- InconsistentDeclarationParameterNameCheck.cpp - clang-tidy-------===//
+//
+//           The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "InconsistentDeclarationParameterNameCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include <algorithm>
+#include <functional>
+#include <sstream>
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+using namespace ast_matchers;
+
+namespace {
+
+AST_MATCHER(FunctionDecl, hasOtherDeclarations) {
+  auto It = Node.redecls_begin();
+  auto EndIt = Node.redecls_end();
+
+  if (It == EndIt)
+    return false;
+
+  ++It;
+  return It != EndIt;
+}
+
+struct DifferingParamInfo {
+  DifferingParamInfo(StringRef SourceName, StringRef OtherName,
+                     SourceRange OtherNameRange, bool GenerateFixItHint)
+      : SourceName(SourceName), OtherName(OtherName),
+        OtherNameRange(OtherNameRange), GenerateFixItHint(GenerateFixItHint) {}
+
+  StringRef SourceName;
+  StringRef OtherName;
+  SourceRange OtherNameRange;
+  bool GenerateFixItHint;
+};
+
+using DifferingParamsContainer = llvm::SmallVector<DifferingParamInfo, 10>;
+
+struct InconsistentDeclarationInfo {
+  InconsistentDeclarationInfo(SourceLocation DeclarationLocation,
+                              DifferingParamsContainer &&DifferingParams)
+      : DeclarationLocation(DeclarationLocation),
+        DifferingParams(std::move(DifferingParams)) {}
+
+  SourceLocation DeclarationLocation;
+  DifferingParamsContainer DifferingParams;
+};
+
+using InconsistentDeclarationsContainer =
+    llvm::SmallVector<InconsistentDeclarationInfo, 2>;
+
+bool checkIfFixItHintIsApplicable(
+    const FunctionDecl *ParameterSourceDeclaration,
+    const ParmVarDecl *SourceParam, const FunctionDecl *OriginalDeclaration) {
+  // Assumptions with regard to function declarations/definition:
+  //  * If both function declaration and definition are seen, assume that
+  //  definition is most up-to-date, and use it to generate replacements.
+  //  * If only function declarations are seen, there is no easy way to tell
+  //  which is up-to-date and which is not, so don't do anything.
+  // TODO: This may be changed later, but for now it seems the reasonable
+  // solution.
+  if (!ParameterSourceDeclaration->isThisDeclarationADefinition())
+    return false;
+
+  // Assumption: if parameter is not referenced in function defintion body, it
+  // may indicate that it's outdated, so don't touch it.
+  if (!SourceParam->isReferenced())
+    return false;
+
+  // In case there is the primary template definition and (possibly several)
+  // template specializations (and each with possibly several redeclarations),
+  // it is not at all clear what to change.
+  if (OriginalDeclaration->getTemplatedKind() ==
+      FunctionDecl::TK_FunctionTemplateSpecialization)
+    return false;
+
+  // Other cases seem OK to allow replacements.
+  return true;
+}
+
+DifferingParamsContainer
+findDifferingParamsInDeclaration(const FunctionDecl *ParameterSourceDeclaration,
+                                 const FunctionDecl *OtherDeclaration,
+                                 const FunctionDecl *OriginalDeclaration) {
+  DifferingParamsContainer DifferingParams;
+
+  auto SourceParamIt = ParameterSourceDeclaration->param_begin();
+  auto OtherParamIt = OtherDeclaration->param_begin();
+
+  while (SourceParamIt != ParameterSourceDeclaration->param_end() &&
+         OtherParamIt != OtherDeclaration->param_end()) {
+    auto SourceParamName = (*SourceParamIt)->getName();
+    auto OtherParamName = (*OtherParamIt)->getName();
+
+    // FIXME: Provide a way to extract commented out parameter name from comment
+    // next to it.
+    if (!SourceParamName.empty() && !OtherParamName.empty() &&
+        SourceParamName != OtherParamName) {
+      SourceRange OtherParamNameRange =
+          DeclarationNameInfo((*OtherParamIt)->getDeclName(),
+                              (*OtherParamIt)->getLocation()).getSourceRange();
+
+      bool GenerateFixItHint = checkIfFixItHintIsApplicable(
+          ParameterSourceDeclaration, *SourceParamIt, OriginalDeclaration);
+
+      DifferingParams.emplace_back(SourceParamName, OtherParamName,
+                                   OtherParamNameRange, GenerateFixItHint);
+    }
+
+    ++SourceParamIt;
+    ++OtherParamIt;
+  }
+
+  return DifferingParams;
+}
+
+InconsistentDeclarationsContainer
+findInconsitentDeclarations(const FunctionDecl *OriginalDeclaration,
+                            const FunctionDecl *ParameterSourceDeclaration,
+                            SourceManager &SM) {
+  InconsistentDeclarationsContainer InconsistentDeclarations;
+  SourceLocation ParameterSourceLocation =
+      ParameterSourceDeclaration->getLocation();
+
+  for (const FunctionDecl *OtherDeclaration : OriginalDeclaration->redecls()) {
+    SourceLocation OtherLocation = OtherDeclaration->getLocation();
+    if (OtherLocation != ParameterSourceLocation) { // Skip self.
+      DifferingParamsContainer DifferingParams =
+          findDifferingParamsInDeclaration(ParameterSourceDeclaration,
+                                           OtherDeclaration,
+                                           OriginalDeclaration);
+      if (!DifferingParams.empty()) {
+        InconsistentDeclarations.emplace_back(OtherDeclaration->getLocation(),
+                                              std::move(DifferingParams));
+      }
+    }
+  }
+
+  // Sort in order of appearance in translation unit to generate clear
+  // diagnostics.
+  std::sort(InconsistentDeclarations.begin(), InconsistentDeclarations.end(),
+            [&SM](const InconsistentDeclarationInfo &Info1,
+                  const InconsistentDeclarationInfo &Info2) {
+              return SM.isBeforeInTranslationUnit(Info1.DeclarationLocation,
+                                                  Info2.DeclarationLocation);
+            });
+  return InconsistentDeclarations;
+}
+
+const FunctionDecl *
+getParameterSourceDeclaration(const FunctionDecl *OriginalDeclaration) {
+  const FunctionTemplateDecl *PrimaryTemplate =
+      OriginalDeclaration->getPrimaryTemplate();
+  if (PrimaryTemplate != nullptr) {
+    // In case of template specializations, use primary template declaration as
+    // the source of parameter names.
+    return PrimaryTemplate->getTemplatedDecl();
+  }
+
+  // In other cases, try to change to function definition, if available.
+
+  if (OriginalDeclaration->isThisDeclarationADefinition())
+    return OriginalDeclaration;
+
+  for (const FunctionDecl *OtherDeclaration : OriginalDeclaration->redecls()) {
+    if (OtherDeclaration->isThisDeclarationADefinition()) {
+      return OtherDeclaration;
+    }
+  }
+
+  // No definition found, so return original declaration.
+  return OriginalDeclaration;
+}
+
+std::string joinParameterNames(
+    const DifferingParamsContainer &DifferingParams,
+    std::function<StringRef(const DifferingParamInfo &)> ChooseParamName) {
+  llvm::SmallVector<char, 40> Buffer;
+  llvm::raw_svector_ostream Str(Buffer);
+  bool First = true;
+  for (const DifferingParamInfo &ParamInfo : DifferingParams) {
+    if (First)
+      First = false;
+    else
+      Str << ", ";
+
+    Str << "'" << ChooseParamName(ParamInfo).str() << "'";
+  }
+  return Str.str().str();
+}
+
+void formatDifferingParamsDiagnostic(
+    InconsistentDeclarationParameterNameCheck *Check,
+    const SourceLocation &Location, StringRef OtherDeclarationDescription,
+    const DifferingParamsContainer &DifferingParams) {
+  auto ChooseOtherName =
+      [](const DifferingParamInfo &ParamInfo) { return ParamInfo.OtherName; };
+  auto ChooseSourceName =
+      [](const DifferingParamInfo &ParamInfo) { return ParamInfo.SourceName; };
+
+  auto ParamDiag =
+      Check->diag(Location,
+                  "differing parameters are named here: (%0), in %1: (%2)",
+                  DiagnosticIDs::Level::Note)
+      << joinParameterNames(DifferingParams, ChooseOtherName)
+      << OtherDeclarationDescription
+      << joinParameterNames(DifferingParams, ChooseSourceName);
+
+  for (const DifferingParamInfo &ParamInfo : DifferingParams) {
+    if (ParamInfo.GenerateFixItHint) {
+      ParamDiag << FixItHint::CreateReplacement(
+          CharSourceRange::getTokenRange(ParamInfo.OtherNameRange),
+          ParamInfo.SourceName);
+    }
+  }
+}
+
+void formatDiagnosticsForDeclarations(
+    InconsistentDeclarationParameterNameCheck *Check,
+    const FunctionDecl *ParameterSourceDeclaration,
+    const FunctionDecl *OriginalDeclaration,
+    const InconsistentDeclarationsContainer &InconsistentDeclarations) {
+  Check->diag(
+      OriginalDeclaration->getLocation(),
+      "function %q0 has %1 other declaration%s1 with different parameter names")
+      << OriginalDeclaration
+      << static_cast<int>(InconsistentDeclarations.size());
+  int Count = 1;
+  for (const InconsistentDeclarationInfo &InconsistentDeclaration :
+       InconsistentDeclarations) {
+    Check->diag(InconsistentDeclaration.DeclarationLocation,
+                "the %ordinal0 inconsistent declaration seen here",
+                DiagnosticIDs::Level::Note)
+        << Count;
+
+    formatDifferingParamsDiagnostic(
+        Check, InconsistentDeclaration.DeclarationLocation,
+        "the other declaration", InconsistentDeclaration.DifferingParams);
+
+    ++Count;
+  }
+}
+
+void formatDiagnostics(
+    InconsistentDeclarationParameterNameCheck *Check,
+    const FunctionDecl *ParameterSourceDeclaration,
+    const FunctionDecl *OriginalDeclaration,
+    const InconsistentDeclarationsContainer &InconsistentDeclarations,
+    StringRef FunctionDescription, StringRef ParameterSourceDescription) {
+  for (const InconsistentDeclarationInfo &InconsistentDeclaration :
+       InconsistentDeclarations) {
+    Check->diag(InconsistentDeclaration.DeclarationLocation,
+                "%0 %q1 has a %2 with different parameter names")
+        << FunctionDescription << OriginalDeclaration
+        << ParameterSourceDescription;
+
+    Check->diag(ParameterSourceDeclaration->getLocation(), "the %0 seen here",
+                DiagnosticIDs::Level::Note)
+        << ParameterSourceDescription;
+
+    formatDifferingParamsDiagnostic(
+        Check, InconsistentDeclaration.DeclarationLocation,
+        ParameterSourceDescription, InconsistentDeclaration.DifferingParams);
+  }
+}
+
+} // anonymous namespace
+
+void InconsistentDeclarationParameterNameCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(unless(isImplicit()), hasOtherDeclarations())
+                         .bind("functionDecl"),
+                     this);
+}
+
+void InconsistentDeclarationParameterNameCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *OriginalDeclaration =
+      Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
+
+  if (VisitedDeclarations.count(OriginalDeclaration) > 0)
+    return; // Avoid multiple warnings.
+
+  const FunctionDecl *ParameterSourceDeclaration =
+      getParameterSourceDeclaration(OriginalDeclaration);
+
+  InconsistentDeclarationsContainer InconsistentDeclarations =
+      findInconsitentDeclarations(OriginalDeclaration,
+                                  ParameterSourceDeclaration,
+                                  *Result.SourceManager);
+  if (InconsistentDeclarations.empty()) {
+    // Avoid unnecessary further visits.
+    markRedeclarationsAsVisited(OriginalDeclaration);
+    return;
+  }
+
+  if (OriginalDeclaration->getTemplatedKind() ==
+      FunctionDecl::TK_FunctionTemplateSpecialization) {
+    formatDiagnostics(this, ParameterSourceDeclaration, OriginalDeclaration,
+                      InconsistentDeclarations,
+                      "function template specialization",
+                      "primary template declaration");
+  } else if (ParameterSourceDeclaration->isThisDeclarationADefinition()) {
+    formatDiagnostics(this, ParameterSourceDeclaration, OriginalDeclaration,
+                      InconsistentDeclarations, "function", "definition");
+  } else {
+    formatDiagnosticsForDeclarations(this, ParameterSourceDeclaration,
+                                     OriginalDeclaration,
+                                     InconsistentDeclarations);
+  }
+
+  markRedeclarationsAsVisited(OriginalDeclaration);
+}
+
+void InconsistentDeclarationParameterNameCheck::markRedeclarationsAsVisited(
+    const FunctionDecl *OriginalDeclaration) {
+  for (const FunctionDecl *Redecl : OriginalDeclaration->redecls()) {
+    VisitedDeclarations.insert(Redecl);
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
+#include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
+    CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
+        "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
     CheckFactories.registerCheck<RedundantStringCStrCheck>(
         "readability-redundant-string-cstr");
     CheckFactories.registerCheck<ShrinkToFitCheck>(
         "readability-shrink-to-fit");

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-loop-convert
    modernize-pass-by-value
    modernize-use-nullptr
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
+   readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-shrink-to-fit
    readability-simplify-boolean-expr
 No newline at end of file

docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.rst

+readability-inconsistent-declaration-parameter-name
+===================================================
+
+
+Find function declarations which differ in parameter names.
+
+Example:
+
+.. code:: c++
+
+  // in foo.hpp:
+  void foo(int a, int b, int c);
+
+  // in foo.cpp:
+  void foo(int d, int e, int f); // warning
+
+This check should help to enforce consistency in large projects, where it often happens
+that a definition of function is refactored, changing the parameter names, but its declaration
+in header file is not updated. With this check, we can easily find and correct such inconsistencies,
+keeping declaration and definition always in sync.
+
+Unnamed parameters are allowed and are not taken into account when comparing function declarations, for example:
+
+.. code:: c++
+
+   void foo(int a);
+   void foo(int); // no warning
+
+If there are multiple declarations of same function, only one warning will be generated.
 No newline at end of file

test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp

+// RUN: %python %S/check_clang_tidy.py %s readability-inconsistent-declaration-parameter-name %t
+
+void consistentFunction(int a, int b, int c);
+void consistentFunction(int a, int b, int c);
+void consistentFunction(int a, int b, int /*c*/);
+void consistentFunction(int /*c*/, int /*c*/, int /*c*/);
+
+//////////////////////////////////////////////////////
+
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: function 'inconsistentFunction' has 2 other declarations with different parameter names [readability-inconsistent-declaration-parameter-name]
+void inconsistentFunction(int a, int b, int c);
+// CHECK-MESSAGES: :[[@LINE+2]]:6: note: the 1st inconsistent declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('d', 'e', 'f'), in the other declaration: ('a', 'b', 'c')
+void inconsistentFunction(int d, int e, int f);
+// CHECK-MESSAGES: :[[@LINE+2]]:6: note: the 2nd inconsistent declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('x', 'y', 'z'), in the other declaration: ('a', 'b', 'c')
+void inconsistentFunction(int x, int y, int z);
+
+//////////////////////////////////////////////////////
+
+// CHECK-MESSAGES: :[[@LINE+4]]:6: warning: function 'inconsistentFunctionWithVisibleDefinition' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
+// CHECK-MESSAGES: :[[@LINE+9]]:6: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:6: note: differing parameters are named here: ('a'), in definition: ('c')
+// CHECK-FIXES: void inconsistentFunctionWithVisibleDefinition(int c);
+void inconsistentFunctionWithVisibleDefinition(int a);
+// CHECK-MESSAGES: :[[@LINE+4]]:6: warning: function 'inconsistentFunctionWithVisibleDefinition' has a definition
+// CHECK-MESSAGES: :[[@LINE+4]]:6: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:6: note: differing parameters are named here: ('b'), in definition: ('c')
+// CHECK-FIXES: void inconsistentFunctionWithVisibleDefinition(int c);
+void inconsistentFunctionWithVisibleDefinition(int b);
+void inconsistentFunctionWithVisibleDefinition(int c) { c; }
+
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function 'inconsidentFunctionWithUnreferencedParameterInDefinition' has a definition
+// CHECK-MESSAGES: :[[@LINE+3]]:6: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('a'), in definition: ('b')
+void inconsidentFunctionWithUnreferencedParameterInDefinition(int a);
+void inconsidentFunctionWithUnreferencedParameterInDefinition(int b) {}
+
+//////////////////////////////////////////////////////
+
+struct Struct {
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: function 'Struct::inconsistentFunction' has a definition
+// CHECK-MESSAGES: :[[@LINE+6]]:14: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:8: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: void inconsistentFunction(int b);
+  void inconsistentFunction(int a);
+};
+
+void Struct::inconsistentFunction(int b) { b = 0; }
+
+//////////////////////////////////////////////////////
+
+struct SpecialFunctions {
+// CHECK-MESSAGES: :[[@LINE+4]]:3: warning: function 'SpecialFunctions::SpecialFunctions' has a definition
+// CHECK-MESSAGES: :[[@LINE+12]]:19: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:3: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: SpecialFunctions(int b);
+  SpecialFunctions(int a);
+
+// CHECK-MESSAGES: :[[@LINE+4]]:21: warning: function 'SpecialFunctions::operator=' has a definition
+// CHECK-MESSAGES: :[[@LINE+8]]:37: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:21: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: SpecialFunctions& operator=(const SpecialFunctions& b);
+  SpecialFunctions& operator=(const SpecialFunctions& a);
+};
+
+SpecialFunctions::SpecialFunctions(int b) { b; }
+
+SpecialFunctions& SpecialFunctions::operator=(const SpecialFunctions& b) { b; return *this; }
+
+//////////////////////////////////////////////////////
+
+// CHECK-MESSAGES: :[[@LINE+5]]:6: warning: function 'templateFunctionWithSeparateDeclarationAndDefinition' has a definition
+// CHECK-MESSAGES: :[[@LINE+7]]:6: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+3]]:6: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: void templateFunctionWithSeparateDeclarationAndDefinition(T b);
+template<typename T>
+void templateFunctionWithSeparateDeclarationAndDefinition(T a);
+
+template<typename T>
+void templateFunctionWithSeparateDeclarationAndDefinition(T b) { b; }
+
+//////////////////////////////////////////////////////
+
+template<typename T>
+void templateFunctionWithSpecializations(T a) { a; }
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function template specialization 'templateFunctionWithSpecializations<int>' has a primary template declaration with different parameter names [readability-inconsistent-declaration-parameter-name]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('b'), in primary template declaration: ('a')
+void templateFunctionWithSpecializations(int b) { b; }
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function template specialization 'templateFunctionWithSpecializations<float>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-10]]:6: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('c'), in primary template declaration: ('a')
+void templateFunctionWithSpecializations(float c) { c; }
+
+//////////////////////////////////////////////////////
+
+template<typename T>
+void templateFunctionWithoutDefinitionButWithSpecialization(T a);
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function template specialization 'templateFunctionWithoutDefinitionButWithSpecialization<int>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('b'), in primary template declaration: ('a')
+void templateFunctionWithoutDefinitionButWithSpecialization(int b) { b; }
+
+//////////////////////////////////////////////////////
+
+template<typename T>
+void templateFunctionWithSeparateSpecializationDeclarationAndDefinition(T a);
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function template specialization 'templateFunctionWithSeparateSpecializationDeclarationAndDefinition<int>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('b'), in primary template declaration: ('a')
+void templateFunctionWithSeparateSpecializationDeclarationAndDefinition(int b);
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function template specialization 'templateFunctionWithSeparateSpecializationDeclarationAndDefinition<int>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-10]]:6: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('c'), in primary template declaration: ('a')
+void templateFunctionWithSeparateSpecializationDeclarationAndDefinition(int c) { c; }
+
+//////////////////////////////////////////////////////
+
+template<typename T>
+class ClassTemplate
+{
+public:
+// CHECK-MESSAGES: :[[@LINE+4]]:10: warning: function 'ClassTemplate::functionInClassTemplateWithSeparateDeclarationAndDefinition' has a definition
+// CHECK-MESSAGES: :[[@LINE+7]]:24: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:10: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: void functionInClassTemplateWithSeparateDeclarationAndDefinition(int b);
+    void functionInClassTemplateWithSeparateDeclarationAndDefinition(int a);
+};
+
+template<typename T>
+void ClassTemplate<T>::functionInClassTemplateWithSeparateDeclarationAndDefinition(int b) { b; }
+
+//////////////////////////////////////////////////////
+
+class Class
+{
+public:
+    template<typename T>
+// CHECK-MESSAGES: :[[@LINE+4]]:8: warning: function 'Class::memberFunctionTemplateWithSeparateDeclarationAndDefinition' has a definition
+// CHECK-MESSAGES: :[[@LINE+12]]:13: note: the definition seen here
+// CHECK-MESSAGES: :[[@LINE+2]]:8: note: differing parameters are named here: ('a'), in definition: ('b')
+// CHECK-FIXES: void memberFunctionTemplateWithSeparateDeclarationAndDefinition(T b);
+  void memberFunctionTemplateWithSeparateDeclarationAndDefinition(T a);
+
+  template<typename T>
+  void memberFunctionTemplateWithSpecializations(T a) { a; }
+};
+
+//////////////////////////////////////////////////////
+
+template<typename T>
+void Class::memberFunctionTemplateWithSeparateDeclarationAndDefinition(T b) { b; }
+
+//////////////////////////////////////////////////////
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:13: warning: function template specialization 'Class::memberFunctionTemplateWithSpecializations<int>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-12]]:8: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:13: note: differing parameters are named here: ('b'), in primary template declaration: ('a')
+void Class::memberFunctionTemplateWithSpecializations(int b) { b; }
+
+template<>
+// CHECK-MESSAGES: :[[@LINE+3]]:13: warning: function template specialization 'Class::memberFunctionTemplateWithSpecializations<float>' has a primary template
+// CHECK-MESSAGES: :[[@LINE-18]]:8: note: the primary template declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:13: note: differing parameters are named here: ('c'), in primary template declaration: ('a')
+void Class::memberFunctionTemplateWithSpecializations(float c) { c; }
+
+//////////////////////////////////////////////////////
+
+#define DECLARE_FUNCTION_WITH_PARAM_NAME(function_name, param_name) \
+  void function_name(int param_name)
+
+// CHECK-MESSAGES: :[[@LINE+1]]:34: warning: function 'macroFunction' has 1 other declaration with different parameter names [readability-inconsistent-declaration-parameter-name]
+DECLARE_FUNCTION_WITH_PARAM_NAME(macroFunction, a);
+// CHECK-MESSAGES: :[[@LINE+2]]:34: note: the 1st inconsistent declaration seen here
+// CHECK-MESSAGES: :[[@LINE+1]]:34: note: differing parameters are named here: ('b'), in the other declaration: ('a')
+DECLARE_FUNCTION_WITH_PARAM_NAME(macroFunction, b);