diff --git a/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp --- a/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp @@ -52,10 +52,22 @@ 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); } @@ -64,16 +76,41 @@ void DurationUnnecessaryConversionCheck::check( const MatchFinder::MatchResult &Result) { const auto *OuterCall = Result.Nodes.getNodeAs("call"); - const auto *Arg = Result.Nodes.getNodeAs("arg"); if (isInMacro(Result, OuterCall)) return; + FixItHint Hint; + if (const auto *Binop = Result.Nodes.getNodeAs("binop")) { + const auto *Arg = Result.Nodes.getNodeAs("arg"); + const auto *InnerCall = Result.Nodes.getNodeAs("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("arg")) { + Hint = FixItHint::CreateReplacement( + OuterCall->getSourceRange(), + tooling::fixit::getText(*Arg, *Result.Context)); + } diag(OuterCall->getBeginLoc(), "remove unnecessary absl::Duration conversions") - << FixItHint::CreateReplacement( - OuterCall->getSourceRange(), - tooling::fixit::getText(*Arg, *Result.Context)); + << Hint; } } // namespace abseil diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst --- a/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst @@ -40,6 +40,17 @@ // 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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp @@ -100,6 +100,44 @@ 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); @@ -108,4 +146,6 @@ 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); }