Index: clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tidy/readability/CMakeLists.txt +++ clang-tidy/readability/CMakeLists.txt @@ -6,6 +6,7 @@ ElseAfterReturnCheck.cpp FunctionSizeCheck.cpp IdentifierNamingCheck.cpp + InconsistentDeclarationParameterNameCheck.cpp NamedParameterCheck.cpp NamespaceCommentCheck.cpp ReadabilityTidyModule.cpp Index: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h =================================================================== --- clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h +++ clang-tidy/readability/InconsistentDeclarationParameterNameCheck.h @@ -0,0 +1,45 @@ +//===--- 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 VisitedDeclarations; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENT_DECLARATION_PARAMETER_NAME_H Index: clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp =================================================================== --- clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp +++ clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp @@ -0,0 +1,244 @@ +//===--- 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 +#include +#include + +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 MainName, StringRef OtherName, + SourceRange OtherNameRange) + : MainName(MainName), OtherName(OtherName), + OtherNameRange(OtherNameRange) {} + + StringRef MainName; + StringRef OtherName; + SourceRange OtherNameRange; +}; + +using DifferingParamsContainer = llvm::SmallVector; + +struct InconsistentDeclarationInfo { + InconsistentDeclarationInfo(SourceLocation DeclarationLocation, + bool IsTemplateSpecialization, + DifferingParamsContainer &&DifferingParams) + : DeclarationLocation(DeclarationLocation), + IsTemplateSpecialization(IsTemplateSpecialization), + DifferingParams(std::move(DifferingParams)) {} + + SourceLocation DeclarationLocation; + bool IsTemplateSpecialization; + DifferingParamsContainer DifferingParams; +}; + +using InconsistentDeclarationsContainer = + llvm::SmallVector; + +InconsistentDeclarationsContainer +findInconsitentDeclarations(const FunctionDecl *FunctionDeclaration, + SourceManager &SM) { + InconsistentDeclarationsContainer InconsistentDeclarations; + for (const FunctionDecl *OtherDeclaration : FunctionDeclaration->redecls()) { + if (OtherDeclaration == FunctionDeclaration) // Skip self. + continue; + + auto MainParamIt = FunctionDeclaration->param_begin(); + auto OtherParamIt = OtherDeclaration->param_begin(); + DifferingParamsContainer DifferingParams; + + while (MainParamIt != FunctionDeclaration->param_end() && + OtherParamIt != OtherDeclaration->param_end()) { + auto MainParamName = (*MainParamIt)->getName(); + auto OtherParamName = (*OtherParamIt)->getName(); + + // FIXME: Provide a way to extract commented out parameter name + // from comment next to it. + if (!MainParamName.empty() && !OtherParamName.empty() && + MainParamName != OtherParamName) { + SourceRange OtherParamNameRange = + DeclarationNameInfo((*OtherParamIt)->getDeclName(), + (*OtherParamIt)->getLocation()) + .getSourceRange(); + DifferingParams.emplace_back(MainParamName, OtherParamName, + OtherParamNameRange); + } + + ++MainParamIt; + ++OtherParamIt; + } + + if (!DifferingParams.empty()) { + bool IsTemplateSpecialization = + OtherDeclaration->getTemplatedKind() == + FunctionDecl::TemplatedKind::TK_FunctionTemplateSpecialization; + + SourceLocation OtherLocation; + if (IsTemplateSpecialization) { + // Template specializations need special handling. + // What we actually see here is a generated declaration from the main + // template declaration, but it's regular getLocation() method + // confusingly shows the location of this specialization, + // not main template. + OtherLocation = OtherDeclaration->getPrimaryTemplate()->getLocation(); + } else { + OtherLocation = OtherDeclaration->getLocation(); + } + + InconsistentDeclarations.emplace_back( + OtherLocation, IsTemplateSpecialization, std::move(DifferingParams)); + } + } + + std::sort(InconsistentDeclarations.begin(), InconsistentDeclarations.end(), + [&SM](const InconsistentDeclarationInfo &Info1, + const InconsistentDeclarationInfo &Info2) { + return SM.isBeforeInTranslationUnit(Info1.DeclarationLocation, + Info2.DeclarationLocation); + }); + return InconsistentDeclarations; +} + +const FunctionDecl * +chooseDefinitionIfAvailable(const FunctionDecl *FunctionDeclaration) { + if (FunctionDeclaration->isThisDeclarationADefinition()) + return FunctionDeclaration; // Already a definition. + + for (const FunctionDecl *OtherDeclaration : FunctionDeclaration->redecls()) { + if (OtherDeclaration->isThisDeclarationADefinition()) { + return OtherDeclaration; // Change to definition. + } + } + + return FunctionDeclaration; // No definition found, so return original + // declaration. +} + +std::string joinParameterNames( + const DifferingParamsContainer &DifferingParams, + std::function ParamNameChooser) { + std::ostringstream Str; + bool First = true; + for (const DifferingParamInfo &ParamInfo : DifferingParams) { + if (First) + First = false; + else + Str << ", "; + + Str << "'" << ParamNameChooser(ParamInfo).str() << "'"; + } + return Str.str(); +} + +} // anonymous namespace + +void InconsistentDeclarationParameterNameCheck::registerMatchers( + MatchFinder *Finder) { + Finder->addMatcher(functionDecl(unless(isImplicit()), hasOtherDeclarations()) + .bind("functionDecl"), + this); +} + +void InconsistentDeclarationParameterNameCheck::check( + const MatchFinder::MatchResult &Result) { + const FunctionDecl *FunctionDeclaration = + Result.Nodes.getNodeAs("functionDecl"); + if (FunctionDeclaration == nullptr) + return; + + if (VisitedDeclarations.count(FunctionDeclaration) > 0) + return; // Avoid multiple warnings. + + FunctionDeclaration = chooseDefinitionIfAvailable(FunctionDeclaration); + bool HaveFunctionDefinition = + FunctionDeclaration->isThisDeclarationADefinition(); + + InconsistentDeclarationsContainer InconsistentDeclarations = + findInconsitentDeclarations(FunctionDeclaration, *Result.SourceManager); + if (InconsistentDeclarations.empty()) { + markRedeclarationsAsVisited( + FunctionDeclaration); // Avoid unnecessary further visits. + return; + } + + diag(FunctionDeclaration->getLocation(), "function %q0 has %1 other " + "declaration%s1 with differently " + "named parameters") + << FunctionDeclaration + << static_cast(InconsistentDeclarations.size()); + + int Count = 1; + for (const InconsistentDeclarationInfo &InconsistentDeclaration : + InconsistentDeclarations) { + diag(InconsistentDeclaration.DeclarationLocation, + "%ordinal0 inconsistent declaration seen here", + DiagnosticIDs::Level::Note) + << Count; + + auto ParamDiag = + diag(InconsistentDeclaration.DeclarationLocation, + "differing parameters are named here: (%0), while in compared " + "declaration: (%1)", + DiagnosticIDs::Level::Note) + << joinParameterNames(InconsistentDeclaration.DifferingParams, + [](const DifferingParamInfo &ParamInfo) + -> StringRef { return ParamInfo.OtherName; }) + << joinParameterNames(InconsistentDeclaration.DifferingParams, + [](const DifferingParamInfo &ParamInfo) + -> StringRef { return ParamInfo.MainName; }); + + if (HaveFunctionDefinition && + !InconsistentDeclaration.IsTemplateSpecialization) { + for (const DifferingParamInfo &ParamInfo : + InconsistentDeclaration.DifferingParams) { + ParamDiag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(ParamInfo.OtherNameRange), + ParamInfo.MainName); + } + } + // TODO: Decide what to do if we see only declarations and no definition. + + ++Count; + } + + markRedeclarationsAsVisited(FunctionDeclaration); +} + +void InconsistentDeclarationParameterNameCheck::markRedeclarationsAsVisited( + const FunctionDecl *FunctionDeclaration) { + for (const FunctionDecl *Redecl : FunctionDeclaration->redecls()) { + VisitedDeclarations.insert(Redecl); + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -15,6 +15,7 @@ #include "ElseAfterReturnCheck.h" #include "FunctionSizeCheck.h" #include "IdentifierNamingCheck.h" +#include "InconsistentDeclarationParameterNameCheck.h" #include "NamedParameterCheck.h" #include "RedundantSmartptrGetCheck.h" #include "RedundantStringCStrCheck.h" @@ -38,6 +39,8 @@ "readability-function-size"); CheckFactories.registerCheck( "readability-identifier-naming"); + CheckFactories.registerCheck( + "readability-inconsistent-declaration-parameter-name"); CheckFactories.registerCheck( "readability-named-parameter"); CheckFactories.registerCheck( Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -47,8 +47,9 @@ 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 Index: docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.rst =================================================================== --- docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.rst +++ docs/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.rst @@ -0,0 +1,29 @@ +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 Index: test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp =================================================================== --- test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp +++ test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp @@ -0,0 +1,149 @@ +// 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 differently named parameters [readability-inconsistent-declaration-parameter-name] +void inconsistentFunction(int a, int b, int c); +// CHECK-MESSAGES: :[[@LINE+2]]:6: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('d', 'e', 'f'), while in compared declaration: ('a', 'b', 'c') +void inconsistentFunction(int d, int e, int f); +// CHECK-MESSAGES: :[[@LINE+2]]:6: note: 2nd inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+1]]:6: note: differing parameters are named here: ('x', 'y', 'z'), while in compared declaration: ('a', 'b', 'c') +void inconsistentFunction(int x, int y, int z); + +////////////////////////////////////////////////////// + +// CHECK-MESSAGES: :[[@LINE+5]]:6: warning: function 'inconsistentFunctionWithVisibleDefinition' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:6: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:6: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: void inconsistentFunctionWithVisibleDefinition(int b); +void inconsistentFunctionWithVisibleDefinition(int a); +void inconsistentFunctionWithVisibleDefinition(int b) {} + +////////////////////////////////////////////////////// + +struct Struct { +// CHECK-MESSAGES: :[[@LINE+7]]:14: warning: function 'Struct::inconsistentFunction' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:8: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:8: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: void inconsistentFunction(int b); + void inconsistentFunction(int a); +}; + +void Struct::inconsistentFunction(int b) {} + +////////////////////////////////////////////////////// + +struct SpecialFunctions { +// CHECK-MESSAGES: :[[@LINE+13]]:19: warning: function 'SpecialFunctions::SpecialFunctions' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:3: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:3: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: SpecialFunctions(int b); + SpecialFunctions(int a); + +// CHECK-MESSAGES: :[[@LINE+9]]:37: warning: function 'SpecialFunctions::operator=' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:21: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:21: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: SpecialFunctions& operator=(const SpecialFunctions& b); + SpecialFunctions& operator=(const SpecialFunctions& a); +}; + +SpecialFunctions::SpecialFunctions(int b) {} + +SpecialFunctions& SpecialFunctions::operator=(const SpecialFunctions& b) { return *this; } + +////////////////////////////////////////////////////// + +// CHECK-MESSAGES: :[[@LINE+8]]:6: warning: function 'templateFunctionWithSeparateDeclarationAndDefinition' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+4]]:6: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+3]]:6: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: void templateFunctionWithSeparateDeclarationAndDefinition(T b); +template +void templateFunctionWithSeparateDeclarationAndDefinition(T a); + +template +void templateFunctionWithSeparateDeclarationAndDefinition(T b) {} + +////////////////////////////////////////////////////// + +template +void templateFunctionWithSpecializations(T a) {} + +template<> +// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function 'templateFunctionWithSpecializations' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE-5]]:6: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +void templateFunctionWithSpecializations(int b) {} + +template<> +// CHECK-MESSAGES: :[[@LINE+3]]:6: warning: function 'templateFunctionWithSpecializations' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE-10]]:6: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE-11]]:6: note: differing parameters are named here: ('a'), while in compared declaration: ('c') +void templateFunctionWithSpecializations(float c) {} + +////////////////////////////////////////////////////// + +template +class ClassTemplate +{ +public: +// CHECK-MESSAGES: :[[@LINE+8]]:24: warning: function 'ClassTemplate::functionInClassTemplateWithSeparateDeclarationAndDefinition' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:10: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:10: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: void functionInClassTemplateWithSeparateDeclarationAndDefinition(int b); + void functionInClassTemplateWithSeparateDeclarationAndDefinition(int a); +}; + +template +void ClassTemplate::functionInClassTemplateWithSeparateDeclarationAndDefinition(int b) {} + +////////////////////////////////////////////////////// + +class Class +{ +public: + template +// CHECK-MESSAGES: :[[@LINE+13]]:13: warning: function 'Class::memberFunctionTemplateWithSeparateDeclarationAndDefinition' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE+3]]:8: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+2]]:8: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +// CHECK-FIXES: void memberFunctionTemplateWithSeparateDeclarationAndDefinition(T b); + void memberFunctionTemplateWithSeparateDeclarationAndDefinition(T a); + + template + void memberFunctionTemplateWithSpecializations(T a) {} +}; + +////////////////////////////////////////////////////// + +template +void Class::memberFunctionTemplateWithSeparateDeclarationAndDefinition(T b) {} + +////////////////////////////////////////////////////// + +template<> +// CHECK-MESSAGES: :[[@LINE+3]]:13: warning: function 'Class::memberFunctionTemplateWithSpecializations' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE-12]]:8: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE-13]]:8: note: differing parameters are named here: ('a'), while in compared declaration: ('b') +void Class::memberFunctionTemplateWithSpecializations(int b) {} + +template<> +// CHECK-MESSAGES: :[[@LINE+3]]:13: warning: function 'Class::memberFunctionTemplateWithSpecializations' has 1 other declaration with differently named parameters [readability-inconsistent-declaration-parameter-name] +// CHECK-MESSAGES: :[[@LINE-18]]:8: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE-19]]:8: note: differing parameters are named here: ('a'), while in compared declaration: ('c') +void Class::memberFunctionTemplateWithSpecializations(float 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 differently named parameters [readability-inconsistent-declaration-parameter-name] +DECLARE_FUNCTION_WITH_PARAM_NAME(macroFunction, a); +// CHECK-MESSAGES: :[[@LINE+2]]:34: note: 1st inconsistent declaration seen here +// CHECK-MESSAGES: :[[@LINE+1]]:34: note: differing parameters are named here: ('b'), while in compared declaration: ('a') +DECLARE_FUNCTION_WITH_PARAM_NAME(macroFunction, b);