hwright updated this revision to Diff 190456.
hwright marked 4 inline comments as done.
hwright added a comment.
Addressed reviewer comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59183/new/
https://reviews.llvm.org/D59183
Files:
clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
test/clang-tidy/Inputs/absl/time/time.h
test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
Index: test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
===================================================================
--- test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
+++ test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
@@ -8,42 +8,80 @@
// 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)
@@ -66,4 +104,8 @@
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)));
}
Index: test/clang-tidy/Inputs/absl/time/time.h
===================================================================
--- test/clang-tidy/Inputs/absl/time/time.h
+++ test/clang-tidy/Inputs/absl/time/time.h
@@ -21,6 +21,7 @@
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{};
@@ -86,4 +87,6 @@
inline Time operator-(Time lhs, Duration rhs);
inline Duration operator-(Time lhs, Time rhs);
+double FDivDuration(Duration num, Duration den);
+
} // namespace absl
Index: docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
===================================================================
--- docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
+++ docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
@@ -6,7 +6,7 @@
Finds and fixes cases where ``absl::Duration`` values are being converted to
numeric types and back again.
-Examples:
+Floating-point examples:
.. code-block:: c++
@@ -17,6 +17,15 @@
// 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;
@@ -25,6 +34,12 @@
// 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
Index: clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
===================================================================
--- clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
+++ clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
@@ -28,12 +28,35 @@
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);
}
@@ -47,7 +70,8 @@
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));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
