[clang-tools-extra] 3860b2a - [clang-tidy] Update Abseil Duration Conversion check to find more cases.
Hyrum Wright via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 13 09:52:49 PDT 2020
Author: Hyrum Wright
Date: 2020-03-13T12:52:37-04:00
New Revision: 3860b2a0bd09291a276b0590939961dffe67fbb6
URL: https://github.com/llvm/llvm-project/commit/3860b2a0bd09291a276b0590939961dffe67fbb6
DIFF: https://github.com/llvm/llvm-project/commit/3860b2a0bd09291a276b0590939961dffe67fbb6.diff
LOG: [clang-tidy] Update Abseil Duration Conversion check to find more cases.
This change improves the check to handle cases with internal scalar
multiplication.
Differential Revision: https://reviews.llvm.org/D75558
Added:
Modified:
clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
clang-tools-extra/test/clang-tidy/checkers/abseil-duration-unnecessary-conversion.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
index 948feee3c504..28f970e17509 100644
--- a/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
@@ -52,10 +52,22 @@ void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
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::registerMatchers(MatchFinder *Finder) {
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;
+ 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(
+ 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
index 2f978f4d57c0..264c5d08b9d2 100644
--- 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 @@ Integer examples:
// 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
index dde1da906349..9730f6b29b1f 100644
--- 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 @@ void f() {
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 @@ void f() {
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);
}
More information about the cfe-commits
mailing list