Skip to content

Commit 3860b2a

Browse files
author
Hyrum Wright
committed
[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
1 parent a2db388 commit 3860b2a

File tree

3 files changed

+93
-5
lines changed

3 files changed

+93
-5
lines changed

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

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,22 @@ void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
5252
callee(functionDecl(hasName("::absl::FDivDuration"))),
5353
hasArgument(0, expr().bind("arg")), hasArgument(1, factory_matcher));
5454

55+
// Matcher which matches a duration argument being scaled,
56+
// e.g. `absl::ToDoubleSeconds(dur) * 2`
57+
auto scalar_matcher = ignoringImpCasts(
58+
binaryOperator(hasOperatorName("*"),
59+
hasEitherOperand(expr(ignoringParenImpCasts(
60+
callExpr(callee(functionDecl(hasAnyName(
61+
FloatConversion, IntegerConversion))),
62+
hasArgument(0, expr().bind("arg")))
63+
.bind("inner_call")))))
64+
.bind("binop"));
65+
5566
Finder->addMatcher(
5667
callExpr(callee(functionDecl(hasName(DurationFactory))),
5768
hasArgument(0, anyOf(inverse_function_matcher,
58-
division_operator_matcher, fdiv_matcher)))
69+
division_operator_matcher, fdiv_matcher,
70+
scalar_matcher)))
5971
.bind("call"),
6072
this);
6173
}
@@ -64,16 +76,41 @@ void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
6476
void DurationUnnecessaryConversionCheck::check(
6577
const MatchFinder::MatchResult &Result) {
6678
const auto *OuterCall = Result.Nodes.getNodeAs<Expr>("call");
67-
const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
6879

6980
if (isInMacro(Result, OuterCall))
7081
return;
7182

83+
FixItHint Hint;
84+
if (const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop")) {
85+
const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
86+
const auto *InnerCall = Result.Nodes.getNodeAs<Expr>("inner_call");
87+
const Expr *LHS = Binop->getLHS();
88+
const Expr *RHS = Binop->getRHS();
89+
90+
if (LHS->IgnoreParenImpCasts() == InnerCall) {
91+
Hint = FixItHint::CreateReplacement(
92+
OuterCall->getSourceRange(),
93+
(llvm::Twine(tooling::fixit::getText(*Arg, *Result.Context)) + " * " +
94+
tooling::fixit::getText(*RHS, *Result.Context))
95+
.str());
96+
} else {
97+
assert(RHS->IgnoreParenImpCasts() == InnerCall &&
98+
"Inner call should be find on the RHS");
99+
100+
Hint = FixItHint::CreateReplacement(
101+
OuterCall->getSourceRange(),
102+
(llvm::Twine(tooling::fixit::getText(*LHS, *Result.Context)) + " * " +
103+
tooling::fixit::getText(*Arg, *Result.Context))
104+
.str());
105+
}
106+
} else if (const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg")) {
107+
Hint = FixItHint::CreateReplacement(
108+
OuterCall->getSourceRange(),
109+
tooling::fixit::getText(*Arg, *Result.Context));
110+
}
72111
diag(OuterCall->getBeginLoc(),
73112
"remove unnecessary absl::Duration conversions")
74-
<< FixItHint::CreateReplacement(
75-
OuterCall->getSourceRange(),
76-
tooling::fixit::getText(*Arg, *Result.Context));
113+
<< Hint;
77114
}
78115

79116
} // namespace abseil

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,17 @@ Integer examples:
4040
// Suggestion - Remove division and conversion
4141
absl::Duration d2 = d1;
4242

43+
Unwrapping scalar operations:
44+
45+
.. code-block:: c++
46+
47+
// Original - Multiplication by a scalar
48+
absl::Duration d1;
49+
absl::Duration d2 = absl::Seconds(absl::ToInt64Seconds(d1) * 2);
50+
51+
// Suggestion - Remove unnecessary conversion
52+
absl::Duration d2 = d1 * 2;
53+
4354
Note: Converting to an integer and back to an ``absl::Duration`` might be a
4455
truncating operation if the value is not aligned to the scale of conversion.
4556
In the rare case where this is the intended result, callers should use

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,44 @@ void f() {
100100
d2 = VALUE(d1);
101101
#undef VALUE
102102

103+
// Multiplication
104+
d2 = absl::Nanoseconds(absl::ToDoubleNanoseconds(d1) * 2);
105+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
106+
// CHECK-FIXES: d2 = d1 * 2
107+
d2 = absl::Microseconds(absl::ToInt64Microseconds(d1) * 2);
108+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
109+
// CHECK-FIXES: d2 = d1 * 2
110+
d2 = absl::Milliseconds(absl::ToDoubleMilliseconds(d1) * 2);
111+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
112+
// CHECK-FIXES: d2 = d1 * 2
113+
d2 = absl::Seconds(absl::ToInt64Seconds(d1) * 2);
114+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
115+
// CHECK-FIXES: d2 = d1 * 2
116+
d2 = absl::Minutes(absl::ToDoubleMinutes(d1) * 2);
117+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
118+
// CHECK-FIXES: d2 = d1 * 2
119+
d2 = absl::Hours(absl::ToInt64Hours(d1) * 2);
120+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
121+
// CHECK-FIXES: d2 = d1 * 2
122+
d2 = absl::Nanoseconds(2 * absl::ToDoubleNanoseconds(d1));
123+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
124+
// CHECK-FIXES: d2 = 2 * d1
125+
d2 = absl::Microseconds(2 * absl::ToInt64Microseconds(d1));
126+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
127+
// CHECK-FIXES: d2 = 2 * d1
128+
d2 = absl::Milliseconds(2 * absl::ToDoubleMilliseconds(d1));
129+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
130+
// CHECK-FIXES: d2 = 2 * d1
131+
d2 = absl::Seconds(2 * absl::ToInt64Seconds(d1));
132+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
133+
// CHECK-FIXES: d2 = 2 * d1
134+
d2 = absl::Minutes(2 * absl::ToDoubleMinutes(d1));
135+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
136+
// CHECK-FIXES: d2 = 2 * d1
137+
d2 = absl::Hours(2 * absl::ToInt64Hours(d1));
138+
// CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
139+
// CHECK-FIXES: d2 = 2 * d1
140+
103141
// These should not match
104142
d2 = absl::Seconds(absl::ToDoubleMilliseconds(d1));
105143
d2 = absl::Seconds(4);
@@ -108,4 +146,6 @@ void f() {
108146
d2 = absl::Seconds(d1 / absl::Seconds(30));
109147
d2 = absl::Hours(absl::FDivDuration(d1, absl::Minutes(1)));
110148
d2 = absl::Milliseconds(absl::FDivDuration(d1, absl::Milliseconds(20)));
149+
d2 = absl::Seconds(absl::ToInt64Milliseconds(d1) * 2);
150+
d2 = absl::Milliseconds(absl::ToDoubleSeconds(d1) * 2);
111151
}

0 commit comments

Comments
 (0)