[clang-tidy] readability-inconsistent-declaration-parameter-name: accept approximate name matches.

[sammccall]

Summary

The goal is to reduce false positives when the difference is intentional, like:

foo(StringRef name);
foo(StringRef name_ref) {

string name = cleanup(name_ref);
...

}

Or semantically unimportant, like:
foo(StringRef full_name);
foo(StringRef name) { ... }

There are other matching names we won't recognise (e.g. syns vs synonyms) but
this catches many that we see in practice, and gives people a systematic
workaround.

[sammccall]

Make strictness (old behavior) available as an option.

[sammccall]

(First foray into clang-tidy, not sure what I'm doing...)

[hokein]

clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

96 ↗

(On Diff #155337)

I have a concern that this is too general -- it will cover the middle case like inBFix in you test, which seems not happen often in the real world.

Two important cases are prefix and suffix, maybe just restrict to these two cases?

[sammccall]

Restrict matches to prefix/suffix, not substring.

[hokein]

LGTM.

clang-tools-extra/trunk/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h

 /// 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),
-        IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", 1) != 0) {}
+        IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", 1) != 0),
+        Strict(Options.getLocalOrGlobal("Strict", 0) != 0) {}
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   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;
   const bool IgnoreMacros;
+  const bool Strict;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENT_DECLARATION_PARAMETER_NAME_H

clang-tools-extra/trunk/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp

bool checkIfFixItHintIsApplicable(
   if (OriginalDeclaration->getTemplatedKind() ==
       FunctionDecl::TK_FunctionTemplateSpecialization)
     return false;
 
   // Other cases seem OK to allow replacements.
   return true;
 }
 
+bool nameMatch(StringRef L, StringRef R, bool Strict) {
+  if (Strict)
+    return L.empty() || R.empty() || L == R;
+  // We allow two names if one is a prefix/suffix of the other, ignoring case.
+  // Important special case: this is true if either parameter has no name!
+  return L.startswith_lower(R) || R.startswith_lower(L) ||
+         L.endswith_lower(R) || R.endswith_lower(L);
+}
+
 DifferingParamsContainer
 findDifferingParamsInDeclaration(const FunctionDecl *ParameterSourceDeclaration,
                                  const FunctionDecl *OtherDeclaration,
-                                 const FunctionDecl *OriginalDeclaration) {
+                                 const FunctionDecl *OriginalDeclaration,
+                                 bool Strict) {
   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) {
+    if (!nameMatch(SourceParamName, OtherParamName, Strict)) {
       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,
+findInconsistentDeclarations(const FunctionDecl *OriginalDeclaration,
                             const FunctionDecl *ParameterSourceDeclaration,
-                            SourceManager &SM) {
+                            SourceManager &SM, bool Strict) {
   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);
+                                           OriginalDeclaration, Strict);
       if (!DifferingParams.empty()) {
         InconsistentDeclarations.emplace_back(OtherDeclaration->getLocation(),
                                               std::move(DifferingParams));
       }
     }
   }
 
   // Sort in order of appearance in translation unit to generate clear
void formatDiagnostics(
   }
 }
 
 } // anonymous namespace
 
 void InconsistentDeclarationParameterNameCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+  Options.store(Opts, "Strict", Strict);
 }
 
 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,
+      findInconsistentDeclarations(OriginalDeclaration,
                                    ParameterSourceDeclaration,
-                                  *Result.SourceManager);
+                                   *Result.SourceManager, Strict);
   if (InconsistentDeclarations.empty()) {
     // Avoid unnecessary further visits.
     markRedeclarationsAsVisited(OriginalDeclaration);
     return;
   }
 
   SourceLocation StartLoc = OriginalDeclaration->getLocStart();
   if (StartLoc.isMacroID() && IgnoreMacros) {

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

 Unnamed parameters are allowed and are not taken into account when comparing
 function declarations, for example:
 
 .. code-block:: c++
 
   void foo(int a);
   void foo(int); // no warning
 
+One name is also allowed to be a case-insensitive prefix/suffix of the other:
+
+.. code-block:: c++
+
+  void foo(int count);
+  void foo(int count_input) { // no warning
+    int count = adjustCount(count_input);
+  }
+
 To help with refactoring, in some cases fix-it hints are generated to align
 parameter names to a single naming convention. This works with the assumption
 that the function definition is the most up-to-date version, as it directly
 references parameter names in its body. Example:
 
 .. code-block:: c++
 
   void foo(int a); // warning and fix-it hint (replace "a" to "b")
   int foo(int b) { return b + 2; } // definition with use of "b"
 
 In the case of multiple redeclarations or function template specializations,
 a warning is issued for every redeclaration or specialization inconsistent with
 the definition or the first declaration seen in a translation unit.
 
 .. option:: IgnoreMacros
 
    If this option is set to non-zero (default is `1`), the check will not warn
    about names declared inside macros.
+
+.. option:: Strict
+
+   If this option is set to non-zero (default is `0`), then names must match
+   exactly (or be absent).

clang-tools-extra/trunk/test/clang-tidy/readability-inconsistent-declaration-parameter-name-strict.cpp

+// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- \
+// RUN:   -config="{CheckOptions: [{key: readability-inconsistent-declaration-parameter-name.Strict, value: 1}]}" \
+// RUN:   -- -std=c++11
+
+void inconsistentFunction(int a, int b, int c);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'inconsistentFunction' has 1 other declaration with different parameter names
+void inconsistentFunction(int prefixA, int b, int cSuffix);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: note: the 1st inconsistent declaration seen here
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: differing parameters are named here: ('prefixA', 'cSuffix'), in the other declaration: ('a', 'c')
+void inconsistentFunction(int a, int b, int c);
+void inconsistentFunction(int /*c*/, int /*c*/, int /*c*/);

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

 // RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- -- -std=c++11 -fno-delayed-template-parsing
 
 void consistentFunction(int a, int b, int c);
 void consistentFunction(int a, int b, int c);
+void consistentFunction(int prefixA, int b, int cSuffix);
+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