Author: hwright Date: Wed Dec 19 08:03:34 2018 New Revision: 349636 URL: http://llvm.org/viewvc/llvm-project?rev=349636&view=rev Log: [clang-tidy] Diagnose abseil-duration-comparison on macro arguments
Summary: This change relaxes the requirements on the utility `rewriteExprFromNumberToDuration` function, and introduces new checking inside of the `abseil-duration-comparison` check to allow macro argument expression transformation. Differential Revision: https://reviews.llvm.org/D55784 Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=349636&r1=349635&r2=349636&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Wed Dec 19 08:03:34 2018 @@ -19,6 +19,26 @@ namespace clang { namespace tidy { namespace abseil { +/// Return `true` if `E` is a either: not a macro at all; or an argument to +/// one. In the latter case, we should still transform it. +static bool IsValidMacro(const MatchFinder::MatchResult &Result, + const Expr *E) { + if (!E->getBeginLoc().isMacroID()) + return true; + + SourceLocation Loc = E->getBeginLoc(); + // We want to get closer towards the initial macro typed into the source only + // if the location is being expanded as a macro argument. + while (Result.SourceManager->isMacroArgExpansion(Loc)) { + // We are calling getImmediateMacroCallerLoc, but note it is essentially + // equivalent to calling getImmediateSpellingLoc in this context according + // to Clang implementation. We are not calling getImmediateSpellingLoc + // because Clang comment says it "should not generally be used by clients." + Loc = Result.SourceManager->getImmediateMacroCallerLoc(Loc); + } + return !Loc.isMacroID(); +} + void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) { auto Matcher = binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="), @@ -35,10 +55,6 @@ void DurationComparisonCheck::registerMa void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) { const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop"); - // Don't try to replace things inside of macro definitions. - if (Binop->getExprLoc().isMacroID()) - return; - llvm::Optional<DurationScale> Scale = getScaleForInverse( Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName()); if (!Scale) @@ -48,19 +64,19 @@ void DurationComparisonCheck::check(cons // want to handle the case of rewriting both sides. This is much simpler if // we unconditionally try and rewrite both, and let the rewriter determine // if nothing needs to be done. - llvm::Optional<std::string> LhsReplacement = + if (!IsValidMacro(Result, Binop->getLHS()) || + !IsValidMacro(Result, Binop->getRHS())) + return; + std::string LhsReplacement = rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS()); - llvm::Optional<std::string> RhsReplacement = + std::string RhsReplacement = rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS()); - if (!(LhsReplacement && RhsReplacement)) - return; - diag(Binop->getBeginLoc(), "perform comparison in the duration domain") << FixItHint::CreateReplacement(Binop->getSourceRange(), - (llvm::Twine(*LhsReplacement) + " " + + (llvm::Twine(LhsReplacement) + " " + Binop->getOpcodeStr() + " " + - *RhsReplacement) + RhsReplacement) .str()); } Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp?rev=349636&r1=349635&r2=349636&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp Wed Dec 19 08:03:34 2018 @@ -183,14 +183,11 @@ llvm::Optional<DurationScale> getScaleFo return ScaleIter->second; } -llvm::Optional<std::string> rewriteExprFromNumberToDuration( +std::string rewriteExprFromNumberToDuration( const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale, const Expr *Node) { const Expr &RootNode = *Node->IgnoreParenImpCasts(); - if (RootNode.getBeginLoc().isMacroID()) - return llvm::None; - // First check to see if we can undo a complimentary function call. if (llvm::Optional<std::string> MaybeRewrite = rewriteInverseDurationCall(Result, Scale, RootNode)) Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h?rev=349636&r1=349635&r2=349636&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (original) +++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Wed Dec 19 08:03:34 2018 @@ -65,7 +65,7 @@ llvm::Optional<DurationScale> getScaleFo /// Assuming `Node` has type `double` or `int` representing a time interval of /// `Scale`, return the expression to make it a suitable `Duration`. -llvm::Optional<std::string> rewriteExprFromNumberToDuration( +std::string rewriteExprFromNumberToDuration( const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale, const Expr *Node); Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp?rev=349636&r1=349635&r2=349636&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp Wed Dec 19 08:03:34 2018 @@ -42,10 +42,8 @@ void DurationSubtractionCheck::check(con if (!Scale) return; - llvm::Optional<std::string> RhsReplacement = + std::string RhsReplacement = rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS()); - if (!RhsReplacement) - return; const Expr *LhsArg = Result.Nodes.getNodeAs<Expr>("lhs_arg"); @@ -54,7 +52,7 @@ void DurationSubtractionCheck::check(con Binop->getSourceRange(), (llvm::Twine("absl::") + FuncDecl->getName() + "(" + tooling::fixit::getText(*LhsArg, *Result.Context) + " - " + - *RhsReplacement + ")") + RhsReplacement + ")") .str()); } Modified: clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp?rev=349636&r1=349635&r2=349636&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/abseil-duration-comparison.cpp Wed Dec 19 08:03:34 2018 @@ -127,6 +127,33 @@ void f() { // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison] // CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1; + // We should still transform the expression inside this macro invocation +#define VALUE_IF(v, e) v ? (e) : 0 + int a = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1)); + // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1); +#undef VALUE_IF + +#define VALUE_IF_2(e) (e) +#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0) + int a2 = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1)); + // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1); +#undef VALUE_IF +#undef VALUE_IF_2 + +#define VALUE_IF_2(e) (e) +#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0) + int a3 = VALUE_IF(1, d1, Double); +#undef VALUE_IF +#undef VALUE_IF_2 + +#define VALUE_IF_2(e) (e) +#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0) + int a4 = VALUE_IF(1, d1, Double); +#undef VALUE_IF +#undef VALUE_IF_2 + // These should not match b = 6 < 4; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
