[clang-tidy] Expand cases covered by the abseil-duration-unnecessary-conversion check

[hwright]

Summary

This adds coverage for expressions of these forms

absl::Duration d2, d1;
d2 = absl::Seconds(d1 / absl::Seconds(1));
d2 = absl::Seconds(absl::FDivDuration(d1, absl::Seconds(1)));

[hokein]

clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

31

could you add a few comment briefly describing these matchers?

test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

86

An off-topic comment: do we have this code pattern in the codebase? From my understanding, the usage like this is really rare.

88

could you check the full statement d2 = d1? the same to other places.

[hwright]

Addressed reviewer comments

test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

86

This mostly shows up in migrations. When an existing API takes an integer, and caller already has a Duration, they sometimes use dur / absl::Seconds(1) to get back in integer at the callsite. After a migration, we end up with absl::Seconds(dur /absl::Seconds(1)), which should really just be dur.

clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
   for (const auto &Scale : {"Hours", "Minutes", "Seconds", "Milliseconds",
                             "Microseconds", "Nanoseconds"}) {
     std::string DurationFactory = (llvm::Twine("::absl::") + Scale).str();
     std::string FloatConversion =
         (llvm::Twine("::absl::ToDouble") + Scale).str();
     std::string IntegerConversion =
         (llvm::Twine("::absl::ToInt64") + Scale).str();
 
+    // Matcher which matches the current scale's factory with a `1` argument,
+    // e.g. `absl::Seconds(1)`.
+    auto factory_matcher = cxxConstructExpr(hasArgument(
+        0,
+        callExpr(callee(functionDecl(hasName(DurationFactory))),
+                 hasArgument(0, ignoringImpCasts(integerLiteral(equals(1)))))));
+
+    // Matcher which matches either inverse function and binds its argument,
+    // e.g. `absl::ToDoubleSeconds(dur)`.
+    auto inverse_function_matcher = callExpr(
+        callee(functionDecl(hasAnyName(FloatConversion, IntegerConversion))),
+        hasArgument(0, expr().bind("arg")));
+
+    // Matcher which matches a duration divided by the factory_matcher above,
+    // e.g. `dur / absl::Seconds(1)`.
+    auto division_operator_matcher = cxxOperatorCallExpr(
+        hasOverloadedOperatorName("/"), hasArgument(0, expr().bind("arg")),
+        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));
+
     Finder->addMatcher(
-        callExpr(
-            callee(functionDecl(hasName(DurationFactory))),
-            hasArgument(0, callExpr(callee(functionDecl(hasAnyName(
-                                        FloatConversion, IntegerConversion))),
-                                    hasArgument(0, expr().bind("arg")))))
+        callExpr(callee(functionDecl(hasName(DurationFactory))),
+                 hasArgument(0, anyOf(inverse_function_matcher,
+                                      division_operator_matcher, fdiv_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")
+  diag(OuterCall->getBeginLoc(),
+       "remove unnecessary absl::Duration conversions")
       << FixItHint::CreateReplacement(
              OuterCall->getSourceRange(),
              tooling::fixit::getText(*Arg, *Result.Context));
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

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

 .. title:: clang-tidy - abseil-duration-unnecessary-conversion
 
 abseil-duration-unnecessary-conversion
 ======================================
 
 Finds and fixes cases where ``absl::Duration`` values are being converted to
 numeric types and back again.
 
-Examples:
+Floating-point examples:
 
 .. code-block:: c++
 
   // Original - Conversion to double and back again
   absl::Duration d1;
   absl::Duration d2 = absl::Seconds(absl::ToDoubleSeconds(d1));
 
   // Suggestion - Remove unnecessary conversions
   absl::Duration d2 = d1;
 
+  // Original - Division to convert to double and back again
+  absl::Duration d2 = absl::Seconds(absl::FDivDuration(d1, absl::Seconds(1)));
+
+  // Suggestion - Remove division and conversion
+  absl::Duration d2 = d1;
+
+Integer examples:
+
+.. code-block:: c++
 
   // Original - Conversion to integer and back again
   absl::Duration d1;
   absl::Duration d2 = absl::Hours(absl::ToInt64Hours(d1));
 
   // Suggestion - Remove unnecessary conversions
   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;
+
 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.

test/clang-tidy/Inputs/absl/time/time.h

public:
   template <typename T> Duration &operator/=(T r);
 
   Duration &operator+(Duration d);
 };
 
 template <typename T> Duration operator*(Duration lhs, T rhs);
 template <typename T> Duration operator*(T lhs, Duration rhs);
 template <typename T> Duration operator/(Duration lhs, T rhs);
+int64_t operator/(Duration lhs, Duration rhs);
 
 class Time{};
 
 constexpr Duration Nanoseconds(long long);
 constexpr Duration Microseconds(long long);
 constexpr Duration Milliseconds(long long);
 constexpr Duration Seconds(long long);
 constexpr Duration Minutes(long long);

 constexpr bool operator!=(Duration lhs, Duration rhs);
 
 // Additive Operators
 inline Time operator+(Time lhs, Duration rhs);
 inline Time operator+(Duration lhs, Time rhs);
 inline Time operator-(Time lhs, Duration rhs);
 inline Duration operator-(Time lhs, Time rhs);
 
+double FDivDuration(Duration num, Duration den);
+
 }  // namespace absl

test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

 // RUN: %check_clang_tidy %s abseil-duration-unnecessary-conversion %t -- -- -I%S/Inputs
 
 #include "absl/time/time.h"
 
 void f() {
   absl::Duration d1, d2;
 
   // Floating point
   d2 = absl::Hours(absl::ToDoubleHours(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Minutes(absl::ToDoubleMinutes(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Seconds(absl::ToDoubleSeconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Milliseconds(absl::ToDoubleMilliseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Microseconds(absl::ToDoubleMicroseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Nanoseconds(absl::ToDoubleNanoseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
 
   // Integer point
   d2 = absl::Hours(absl::ToInt64Hours(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Minutes(absl::ToInt64Minutes(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Seconds(absl::ToInt64Seconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Milliseconds(absl::ToInt64Milliseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Microseconds(absl::ToInt64Microseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Nanoseconds(absl::ToInt64Nanoseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
+
+  d2 = absl::Hours(d1 / absl::Hours(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Minutes(d1 / absl::Minutes(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Seconds(d1 / absl::Seconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Milliseconds(d1 / absl::Milliseconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Microseconds(d1 / absl::Microseconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Nanoseconds(d1 / absl::Nanoseconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+
+  d2 = absl::Hours(absl::FDivDuration(d1, absl::Hours(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Minutes(absl::FDivDuration(d1, absl::Minutes(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Seconds(absl::FDivDuration(d1, absl::Seconds(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Milliseconds(absl::FDivDuration(d1, absl::Milliseconds(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Microseconds(absl::FDivDuration(d1, absl::Microseconds(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Nanoseconds(absl::FDivDuration(d1, absl::Nanoseconds(1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
 
   // As macro argument
 #define PLUS_FIVE_S(x) x + absl::Seconds(5)
   d2 = PLUS_FIVE_S(absl::Seconds(absl::ToInt64Seconds(d1)));
   // CHECK-MESSAGES: [[@LINE-1]]:20: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
   // CHECK-FIXES: PLUS_FIVE_S(d1)
 #undef PLUS_FIVE_S
 
   // Split by macro: should not change
 #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
 
   // 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)));
 }