[clang-tidy] Update abseil-duration-unnecessary-conversion check to find more cases.

[hwright]

Summary

This check now handles cases where there's a scalar multiplication happening between the two conversions.

[dblaikie]

@ymandel Sorry for this inconvenience, but Phabricator has a bug/missing feature where it doesn't send email if a status change (like approving a patch) has no user-authored text. In the future, please include some text ("Thanks!", "Looks good", etc) in any approval to ensure that approval is reflected on the LLVM mailing lists - otherwise it looks like patches are being committed without review/approval.

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

auto division_operator_matcher = cxxOperatorCallExpr(
         hasArgument(1, factory_matcher));
 
     // Matcher which matches a duration argument to `FDivDuration`,
     // e.g. `absl::FDivDuration(dur, absl::Seconds(1))`
     auto fdiv_matcher = callExpr(
         callee(functionDecl(hasName("::absl::FDivDuration"))),
         hasArgument(0, expr().bind("arg")), hasArgument(1, factory_matcher));
 
+    // Matcher which matches a duration argument being scaled,
+    // e.g. `absl::ToDoubleSeconds(dur) * 2`
+    auto scalar_matcher = ignoringImpCasts(
+        binaryOperator(hasOperatorName("*"),
+                       hasEitherOperand(expr(ignoringParenImpCasts(
+                           callExpr(callee(functionDecl(hasAnyName(
+                                        FloatConversion, IntegerConversion))),
+                                    hasArgument(0, expr().bind("arg")))
+                               .bind("inner_call")))))
+            .bind("binop"));
+
     Finder->addMatcher(
         callExpr(callee(functionDecl(hasName(DurationFactory))),
                  hasArgument(0, anyOf(inverse_function_matcher,
-                                      division_operator_matcher, fdiv_matcher)))
+                                      division_operator_matcher, fdiv_matcher,
+                                      scalar_matcher)))
             .bind("call"),
         this);
   }
 }
 
 void DurationUnnecessaryConversionCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *OuterCall = Result.Nodes.getNodeAs<Expr>("call");
-  const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
 
   if (isInMacro(Result, OuterCall))
     return;
 
-  diag(OuterCall->getBeginLoc(),
-       "remove unnecessary absl::Duration conversions")
-      << FixItHint::CreateReplacement(
-             OuterCall->getSourceRange(),
-             tooling::fixit::getText(*Arg, *Result.Context));
+  FixItHint Hint;
+  if (const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop")) {
+    const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+    const auto *InnerCall = Result.Nodes.getNodeAs<Expr>("inner_call");
+    const Expr *LHS = Binop->getLHS();
+    const Expr *RHS = Binop->getRHS();
+
+    if (LHS->IgnoreParenImpCasts() == InnerCall) {
+      Hint = FixItHint::CreateReplacement(
+          OuterCall->getSourceRange(),
+          (llvm::Twine(tooling::fixit::getText(*Arg, *Result.Context)) + " * " +
+           tooling::fixit::getText(*RHS, *Result.Context))
+              .str());
+    } else {
+      assert(RHS->IgnoreParenImpCasts() == InnerCall &&
+             "Inner call should be find on the RHS");
+
+      Hint = FixItHint::CreateReplacement(
+          OuterCall->getSourceRange(),
+          (llvm::Twine(tooling::fixit::getText(*LHS, *Result.Context)) + " * " +
+           tooling::fixit::getText(*Arg, *Result.Context))
+              .str());
+    }
+  } else if (const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg")) {
+    Hint = FixItHint::CreateReplacement(
   }
+  diag(OuterCall->getBeginLoc(),
+       "remove unnecessary absl::Duration conversions")
+      << Hint;
+}
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst

.. code-block:: c++
   absl::Duration d2 = d1;
 
   // Original - Integer division followed by conversion
   absl::Duration d2 = absl::Seconds(d1 / absl::Seconds(1));
 
   // Suggestion - Remove division and conversion
   absl::Duration d2 = d1;
 
+Unwrapping scalar operations:
+
+.. code-block:: c++
+
+  // Original - Multiplication by a scalar
+  absl::Duration d1;
+  absl::Duration d2 = absl::Seconds(absl::ToInt64Seconds(d1) * 2);
+
+  // Suggestion - Remove unnecessary conversion
+  absl::Duration d2 = d1 * 2;
+
 Note: Converting to an integer and back to an ``absl::Duration`` might be a
 truncating operation if the value is not aligned to the scale of conversion.
 In the rare case where this is the intended result, callers should use
 ``absl::Trunc`` to truncate explicitly.

clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp

#define TOSECONDS(x) absl::Seconds(x)
   d2 = TOSECONDS(absl::ToInt64Seconds(d1));
 #undef TOSECONDS
 
   // Don't change something inside a macro definition
 #define VALUE(x) absl::Hours(absl::ToInt64Hours(x));
   d2 = VALUE(d1);
 #undef VALUE
 
+  // Multiplication
+  d2 = absl::Nanoseconds(absl::ToDoubleNanoseconds(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Microseconds(absl::ToInt64Microseconds(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Milliseconds(absl::ToDoubleMilliseconds(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Seconds(absl::ToInt64Seconds(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Minutes(absl::ToDoubleMinutes(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Hours(absl::ToInt64Hours(d1) * 2);
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1 * 2
+  d2 = absl::Nanoseconds(2 * absl::ToDoubleNanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+  d2 = absl::Microseconds(2 * absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+  d2 = absl::Milliseconds(2 * absl::ToDoubleMilliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+  d2 = absl::Seconds(2 * absl::ToInt64Seconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+  d2 = absl::Minutes(2 * absl::ToDoubleMinutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+  d2 = absl::Hours(2 * absl::ToInt64Hours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = 2 * d1
+
   // These should not match
   d2 = absl::Seconds(absl::ToDoubleMilliseconds(d1));
   d2 = absl::Seconds(4);
   int i = absl::ToInt64Milliseconds(d1);
   d2 = absl::Hours(d1 / absl::Minutes(1));
   d2 = absl::Seconds(d1 / absl::Seconds(30));
   d2 = absl::Hours(absl::FDivDuration(d1, absl::Minutes(1)));
   d2 = absl::Milliseconds(absl::FDivDuration(d1, absl::Milliseconds(20)));
+  d2 = absl::Seconds(absl::ToInt64Milliseconds(d1) * 2);
+  d2 = absl::Milliseconds(absl::ToDoubleSeconds(d1) * 2);
 }