[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

llvm-svn: 349636

clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp

@@ -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::registerMatchers(MatchFinder *Finder) {
 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(const MatchFinder::MatchResult &Result) {
   // 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 =
-      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
-  llvm::Optional<std::string> RhsReplacement =
-      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
-
-  if (!(LhsReplacement && RhsReplacement))
+  if (!IsValidMacro(Result, Binop->getLHS()) ||
+      !IsValidMacro(Result, Binop->getRHS()))
     return;
+  std::string LhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
+  std::string RhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
 
   diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
       << FixItHint::CreateReplacement(Binop->getSourceRange(),
-                                      (llvm::Twine(*LhsReplacement) + " " +
+                                      (llvm::Twine(LhsReplacement) + " " +
                                        Binop->getOpcodeStr() + " " +
-                                       *RhsReplacement)
+                                       RhsReplacement)
                                           .str());
 }
 

clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp

@@ -183,14 +183,11 @@ llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
   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))

clang-tools-extra/clang-tidy/abseil/DurationRewriter.h

@@ -65,7 +65,7 @@ llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name);
 
 /// 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);
 

clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.cpp

@@ -42,10 +42,8 @@ void DurationSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
   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(const MatchFinder::MatchResult &Result) {
              Binop->getSourceRange(),
              (llvm::Twine("absl::") + FuncDecl->getName() + "(" +
               tooling::fixit::getText(*LhsArg, *Result.Context) + " - " +
-              *RhsReplacement + ")")
+              RhsReplacement + ")")
                  .str());
 }
 

clang-tools-extra/test/clang-tidy/abseil-duration-comparison.cpp

@@ -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;