Skip to content

Commit

Permalink
[clang-tidy] Add additional patterns to the abseil-duration-unnecessa…
Browse files Browse the repository at this point in the history 
…ry-conversion check.

Differential Revision: https://reviews.llvm.org/D59183

llvm-svn: 356141
  • Loading branch information
Hyrum Wright committed Mar 14, 2019
1 parent dfce2dd commit 4199a73
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 19 deletions.
36 changes: 30 additions & 6 deletions clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
View file
Expand Up @@ -28,12 +28,35 @@ void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
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);
}
Expand All @@ -47,7 +70,8 @@ void DurationUnnecessaryConversionCheck::check(
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));
Expand Down
17 changes: 16 additions & 1 deletion ...g-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
View file
Expand Up @@ -6,7 +6,7 @@ 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++

Expand All @@ -17,6 +17,15 @@ Examples:
// 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;
Expand All @@ -25,6 +34,12 @@ Examples:
// 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
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/test/clang-tidy/Inputs/absl/time/time.h
View file
Expand Up @@ -21,6 +21,7 @@ class Duration {
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{};

Expand Down Expand Up @@ -86,4 +87,6 @@ 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
66 changes: 54 additions & 12 deletions clang-tools-extra/test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
View file
Expand Up @@ -8,42 +8,80 @@ void f() {
// 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)
Expand All @@ -66,4 +104,8 @@ void f() {
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)));
}

0 comments on commit 4199a73

Please sign in to comment.