Author: alexfh Date: Mon Feb 6 09:47:17 2017 New Revision: 294193 URL: http://llvm.org/viewvc/llvm-project?rev=294193&view=rev Log: [clang-tidy] misc-argument-comment support for gmock
Now for real. The use case supported previously is used by approximately nobody. What's needed is support for matching argument comments in EXPECT_xxx calls to the names of parameters of the mocked methods. Added: clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment-gmock.cpp Modified: clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment.cpp Modified: clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp?rev=294193&r1=294192&r2=294193&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp Mon Feb 6 09:47:17 2017 @@ -12,6 +12,7 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Token.h" +#include "../utils/LexerUtils.h" using namespace clang::ast_matchers; @@ -86,8 +87,26 @@ getCommentsInRange(ASTContext *Ctx, Char return Comments; } -bool ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, - StringRef ArgName, unsigned ArgIndex) { +static std::vector<std::pair<SourceLocation, StringRef>> +getCommentsBeforeLoc(ASTContext *Ctx, SourceLocation Loc) { + std::vector<std::pair<SourceLocation, StringRef>> Comments; + while (Loc.isValid()) { + clang::Token Tok = + utils::lexer::getPreviousToken(*Ctx, Loc, /*SkipComments=*/false); + if (Tok.isNot(tok::comment)) + break; + Loc = Tok.getLocation(); + Comments.emplace_back( + Loc, + Lexer::getSourceText(CharSourceRange::getCharRange( + Loc, Loc.getLocWithOffset(Tok.getLength())), + Ctx->getSourceManager(), Ctx->getLangOpts())); + } + return Comments; +} + +static bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, + StringRef ArgName, unsigned ArgIndex) { std::string ArgNameLowerStr = ArgName.lower(); StringRef ArgNameLower = ArgNameLowerStr; // The threshold is arbitrary. @@ -128,72 +147,129 @@ static bool sameName(StringRef InComment return InComment.compare_lower(InDecl) == 0; } +// This uses implementation details of MOCK_METHODx_ macros: for each mocked +// method M it defines M() with appropriate signature and a method used to set +// up expectations - gmock_M() - with each argument's type changed the +// corresponding matcher. This function finds M by gmock_M. +static const CXXMethodDecl * +findMockedMethod(const CXXMethodDecl *ExpectMethod) { + if (!ExpectMethod->getNameInfo().getName().isIdentifier()) + return nullptr; + StringRef Name = ExpectMethod->getName(); + if (!Name.startswith("gmock_")) + return nullptr; + Name = Name.substr(strlen("gmock_")); + + const DeclContext *Ctx = ExpectMethod->getDeclContext(); + if (Ctx == nullptr || !Ctx->isRecord()) + return nullptr; + for (const auto *D : Ctx->decls()) { + if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) { + if (Method->getName() != Name) + continue; + // Sanity check the mocked method. + if (Method->getNextDeclInContext() == ExpectMethod && + Method->getLocation().isMacroID() && + Method->getNumParams() == ExpectMethod->getNumParams()) { + return Method; + } + } + } + return nullptr; +} + +// For gmock expectation builder method (the target of the call generated by +// `EXPECT_CALL(obj, Method(...))`) tries to find the real method being mocked +// (returns nullptr, if the mock method doesn't override anything). For other +// functions returns the function itself. +static const FunctionDecl *resolveMocks(const FunctionDecl *Func) { + if (const auto *Method = dyn_cast<CXXMethodDecl>(Func)) { + if (const auto *MockedMethod = findMockedMethod(Method)) { + // If mocked method overrides the real one, we can use its parameter + // names, otherwise we're out of luck. + if (MockedMethod->size_overridden_methods() > 0) { + return *MockedMethod->begin_overridden_methods(); + } + return nullptr; + } + } + return Func; +} + void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, - const FunctionDecl *Callee, + const FunctionDecl *OriginalCallee, SourceLocation ArgBeginLoc, llvm::ArrayRef<const Expr *> Args) { + const FunctionDecl *Callee = resolveMocks(OriginalCallee); + if (!Callee) + return; + Callee = Callee->getFirstDecl(); - for (unsigned I = 0, - E = std::min<unsigned>(Args.size(), Callee->getNumParams()); - I != E; ++I) { + unsigned NumArgs = std::min<unsigned>(Args.size(), Callee->getNumParams()); + if (NumArgs == 0) + return; + + auto makeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) { + return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End), + Ctx->getSourceManager(), + Ctx->getLangOpts()); + }; + + for (unsigned I = 0; I < NumArgs; ++I) { const ParmVarDecl *PVD = Callee->getParamDecl(I); IdentifierInfo *II = PVD->getIdentifier(); if (!II) continue; if (auto Template = Callee->getTemplateInstantiationPattern()) { // Don't warn on arguments for parameters instantiated from template - // parameter packs. If we find more arguments than the template definition - // has, it also means that they correspond to a parameter pack. + // parameter packs. If we find more arguments than the template + // definition has, it also means that they correspond to a parameter + // pack. if (Template->getNumParams() <= I || Template->getParamDecl(I)->isParameterPack()) { continue; } } - CharSourceRange BeforeArgument = CharSourceRange::getCharRange( - I == 0 ? ArgBeginLoc : Args[I - 1]->getLocEnd(), - Args[I]->getLocStart()); - BeforeArgument = Lexer::makeFileCharRange( - BeforeArgument, Ctx->getSourceManager(), Ctx->getLangOpts()); + CharSourceRange BeforeArgument = + makeFileCharRange(ArgBeginLoc, Args[I]->getLocStart()); + ArgBeginLoc = Args[I]->getLocEnd(); + + std::vector<std::pair<SourceLocation, StringRef>> Comments; + if (BeforeArgument.isValid()) { + Comments = getCommentsInRange(Ctx, BeforeArgument); + } else { + // Fall back to parsing back from the start of the argument. + CharSourceRange ArgsRange = makeFileCharRange( + Args[I]->getLocStart(), Args[NumArgs - 1]->getLocEnd()); + Comments = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin()); + } - for (auto Comment : getCommentsInRange(Ctx, BeforeArgument)) { + for (auto Comment : Comments) { llvm::SmallVector<StringRef, 2> Matches; - if (IdentRE.match(Comment.second, &Matches)) { - if (!sameName(Matches[2], II->getName(), StrictMode)) { - { - DiagnosticBuilder Diag = - diag(Comment.first, "argument name '%0' in comment does not " - "match parameter name %1") - << Matches[2] << II; - if (isLikelyTypo(Callee->parameters(), Matches[2], I)) { - Diag << FixItHint::CreateReplacement( - Comment.first, - (Matches[1] + II->getName() + Matches[3]).str()); - } + if (IdentRE.match(Comment.second, &Matches) && + !sameName(Matches[2], II->getName(), StrictMode)) { + { + DiagnosticBuilder Diag = + diag(Comment.first, "argument name '%0' in comment does not " + "match parameter name %1") + << Matches[2] << II; + if (isLikelyTypo(Callee->parameters(), Matches[2], I)) { + Diag << FixItHint::CreateReplacement( + Comment.first, (Matches[1] + II->getName() + Matches[3]).str()); } - diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) - << II; + } + diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) << II; + if (OriginalCallee != Callee) { + diag(OriginalCallee->getLocation(), + "actual callee (%0) is declared here", DiagnosticIDs::Note) + << OriginalCallee; } } } } } -static const FunctionDecl *resolveMocks(const MatchFinder::MatchResult &Result, - const FunctionDecl *Func) { - if (auto *Method = dyn_cast<CXXMethodDecl>(Func)) { - if (Method->getLocation().isMacroID() && - Lexer::getImmediateMacroName(Method->getLocation(), - *Result.SourceManager, - Result.Context->getLangOpts()) - .contains("MOCK_METHOD") && - Method->size_overridden_methods() != 0) { - Func = *Method->begin_overridden_methods(); - } - } - return Func; -} - void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { const auto *E = Result.Nodes.getNodeAs<Expr>("expr"); if (const auto *Call = dyn_cast<CallExpr>(E)) { @@ -201,12 +277,15 @@ void ArgumentCommentCheck::check(const M if (!Callee) return; - Callee = resolveMocks(Result, Callee); - checkCallArgs(Result.Context, Callee, Call->getCallee()->getLocEnd(), llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs())); } else { const auto *Construct = cast<CXXConstructExpr>(E); + if (Construct->getNumArgs() == 1 && + Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) { + // Ignore implicit construction. + return; + } checkCallArgs( Result.Context, Construct->getConstructor(), Construct->getParenOrBraceRange().getBegin(), Modified: clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h?rev=294193&r1=294192&r2=294193&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h (original) +++ clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.h Mon Feb 6 09:47:17 2017 @@ -43,8 +43,6 @@ private: const bool StrictMode; llvm::Regex IdentRE; - bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, StringRef ArgName, - unsigned ArgIndex); void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee, SourceLocation ArgBeginLoc, llvm::ArrayRef<const Expr *> Args); Added: clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment-gmock.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment-gmock.cpp?rev=294193&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment-gmock.cpp (added) +++ clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment-gmock.cpp Mon Feb 6 09:47:17 2017 @@ -0,0 +1,101 @@ +// RUN: %check_clang_tidy %s misc-argument-comment %t + +namespace testing { +namespace internal { + +template <typename F> +struct Function; + +template <typename R> +struct Function<R()> { + typedef R Result; +}; + +template <typename R, typename A1> +struct Function<R(A1)> + : Function<R()> { + typedef A1 Argument1; +}; + +template <typename R, typename A1, typename A2> +struct Function<R(A1, A2)> + : Function<R(A1)> { + typedef A2 Argument2; +}; + +} // namespace internal + +template <typename F> +class MockSpec { + public: + void f(); +}; + +template <typename T> +class Matcher { + public: + explicit Matcher(); + Matcher(T value); +}; + +} // namespace testing + +#define GMOCK_RESULT_(tn, ...) \ + tn ::testing::internal::Function<__VA_ARGS__>::Result +#define GMOCK_ARG_(tn, N, ...) \ + tn ::testing::internal::Function<__VA_ARGS__>::Argument##N +#define GMOCK_MATCHER_(tn, N, ...) \ + const ::testing::Matcher<GMOCK_ARG_(tn, N, __VA_ARGS__)>& +#define GMOCK_METHOD2_(tn, constness, ct, Method, ...) \ + GMOCK_RESULT_(tn, __VA_ARGS__) \ + ct Method( \ + GMOCK_ARG_(tn, 1, __VA_ARGS__) gmock_a1, \ + GMOCK_ARG_(tn, 2, __VA_ARGS__) gmock_a2) constness; \ + ::testing::MockSpec<__VA_ARGS__> \ + gmock_##Method(GMOCK_MATCHER_(tn, 1, __VA_ARGS__) gmock_a1, \ + GMOCK_MATCHER_(tn, 2, __VA_ARGS__) gmock_a2) constness +#define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__) +#define MOCK_CONST_METHOD2(m, ...) GMOCK_METHOD2_(, const, , m, __VA_ARGS__) +#define GMOCK_EXPECT_CALL_IMPL_(obj, call) \ + ((obj).gmock_##call).f() +#define EXPECT_CALL(obj, call) GMOCK_EXPECT_CALL_IMPL_(obj, call) + +class Base { + public: + virtual void Method(int param_one_base, int param_two_base); +}; +class Derived : public Base { + public: + virtual void Method(int param_one, int param_two); + virtual void Method2(int p_one, int p_two) const; +}; +class MockDerived : public Derived { + public: + MOCK_METHOD2(Method, void(int a, int b)); + MOCK_CONST_METHOD2(Method2, void(int c, int d)); +}; + +class MockStandalone { + public: + MOCK_METHOD2(Method, void(int aaa, int bbb)); +}; + +void test_gmock() { + MockDerived m; + EXPECT_CALL(m, Method(/*param_one=*/1, /*param_tw=*/2)); +// CHECK-MESSAGES: [[@LINE-1]]:42: warning: argument name 'param_tw' in comment does not match parameter name 'param_two' +// CHECK-FIXES: EXPECT_CALL(m, Method(/*param_one=*/1, /*param_two=*/2)); + EXPECT_CALL(m, Method2(/*p_on=*/3, /*p_two=*/4)); +// CHECK-MESSAGES: [[@LINE-1]]:26: warning: argument name 'p_on' in comment does not match parameter name 'p_one' +// CHECK-FIXES: EXPECT_CALL(m, Method2(/*p_one=*/3, /*p_two=*/4)); + + #define PARAM1 11 + #define PARAM2 22 + EXPECT_CALL(m, Method2(/*p_on1=*/PARAM1, /*p_tw2=*/PARAM2)); +// CHECK-MESSAGES: [[@LINE-1]]:26: warning: argument name 'p_on1' in comment does not match parameter name 'p_one' +// CHECK-MESSAGES: [[@LINE-2]]:44: warning: argument name 'p_tw2' in comment does not match parameter name 'p_two' +// CHECK-FIXES: EXPECT_CALL(m, Method2(/*p_one=*/PARAM1, /*p_two=*/PARAM2)); + + MockStandalone m2; + EXPECT_CALL(m2, Method(/*aaa=*/5, /*bbc=*/6)); +} Modified: clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment.cpp?rev=294193&r1=294192&r2=294193&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-argument-comment.cpp Mon Feb 6 09:47:17 2017 @@ -53,59 +53,3 @@ void f(bool _with_underscores_); void ignores_underscores() { f(/*With_Underscores=*/false); } - -// gmock -namespace testing { -namespace internal { - -template <typename F> -struct Function; - -template <typename R> -struct Function<R()> { - typedef R Result; -}; - -template <typename R, typename A1> -struct Function<R(A1)> - : Function<R()> { - typedef A1 Argument1; -}; - -template <typename R, typename A1, typename A2> -struct Function<R(A1, A2)> - : Function<R(A1)> { - typedef A2 Argument2; -}; -} // namespace internal -} // namespace testing - -#define GMOCK_RESULT_(tn, ...) \ - tn ::testing::internal::Function<__VA_ARGS__>::Result -#define GMOCK_ARG_(tn, N, ...) \ - tn ::testing::internal::Function<__VA_ARGS__>::Argument##N -#define GMOCK_METHOD2_(tn, constness, ct, Method, ...) \ - GMOCK_RESULT_(tn, __VA_ARGS__) ct Method( \ - GMOCK_ARG_(tn, 1, __VA_ARGS__) gmock_a1, \ - GMOCK_ARG_(tn, 2, __VA_ARGS__) gmock_a2) constness -#define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__) - -class Base { - public: - virtual void Method(int param_one_base, int param_two_base); -}; -class Derived : public Base { - public: - virtual void Method(int param_one, int param_two); -}; -class MockDerived : public Derived { - public: - MOCK_METHOD2(Method, void(int, int)); -}; - -void test_gmock() { - MockDerived m; - m.Method(/*param_one=*/1, /*param_tw=*/2); -// CHECK-MESSAGES: [[@LINE-1]]:29: warning: argument name 'param_tw' in comment does not match parameter name 'param_two' -// CHECK-FIXES: m.Method(/*param_one=*/1, /*param_two=*/2); -} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits