hwright created this revision.
hwright added reviewers: hokein, ioeric.
Herald added subscribers: cfe-commits, jdoerfert, xazax.hun, mgorny.
Herald added a project: clang.
This is an analog of the `abseil-duration-comparison` check, but for the
`absl::Time` domain. It has a similar implementation and tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D58977
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/TimeComparisonCheck.cpp
clang-tidy/abseil/TimeComparisonCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-time-comparison.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-time-comparison.cpp
Index: test/clang-tidy/abseil-time-comparison.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/abseil-time-comparison.cpp
@@ -0,0 +1,129 @@
+// RUN: %check_clang_tidy %s abseil-time-comparison %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+ double x;
+ absl::Duration d1, d2;
+ bool b;
+ absl::Time t1, t2;
+
+ // Check against the RHS
+ b = x > absl::ToUnixSeconds(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) > t1;
+ b = x >= absl::ToUnixSeconds(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) >= t1;
+ b = x == absl::ToUnixSeconds(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) == t1;
+ b = x <= absl::ToUnixSeconds(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) <= t1;
+ b = x < absl::ToUnixSeconds(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) < t1;
+ b = x == absl::ToUnixSeconds(t1 - d2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixSeconds(x) == t1 - d2;
+ b = absl::ToUnixSeconds(t1) > absl::ToUnixSeconds(t2);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 > t2;
+
+ // Check against the LHS
+ b = absl::ToUnixSeconds(t1) < x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 < absl::FromUnixSeconds(x);
+ b = absl::ToUnixSeconds(t1) <= x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 <= absl::FromUnixSeconds(x);
+ b = absl::ToUnixSeconds(t1) == x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 == absl::FromUnixSeconds(x);
+ b = absl::ToUnixSeconds(t1) >= x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 >= absl::FromUnixSeconds(x);
+ b = absl::ToUnixSeconds(t1) > x;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 > absl::FromUnixSeconds(x);
+
+ // Comparison against zero
+ b = absl::ToUnixSeconds(t1) < 0.0;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 < absl::UnixEpoch();
+ b = absl::ToUnixSeconds(t1) < 0;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: t1 < absl::UnixEpoch();
+
+ // Scales other than Seconds
+ b = x > absl::ToUnixMicros(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixMicros(x) > t1;
+ b = x >= absl::ToUnixMillis(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixMillis(x) >= t1;
+ b = x == absl::ToUnixNanos(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixNanos(x) == t1;
+ b = x <= absl::ToUnixMinutes(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixMinutes(x) <= t1;
+ b = x < absl::ToUnixHours(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixHours(x) < t1;
+
+ // A long expression
+ bool some_condition;
+ int very_very_very_very_long_variable_name;
+ absl::Time SomeTime;
+ if (some_condition && very_very_very_very_long_variable_name
+ < absl::ToUnixSeconds(SomeTime)) {
+ // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: if (some_condition && absl::FromUnixSeconds(very_very_very_very_long_variable_name) < SomeTime) {
+ return;
+ }
+
+ // A complex expression
+ int y;
+ b = (y + 5) * 10 > absl::ToUnixMillis(t1);
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: absl::FromUnixMillis((y + 5) * 10) > t1;
+
+ // We should still transform the expression inside this macro invocation
+#define VALUE_IF(v, e) v ? (e) : 0
+ int a = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+ // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
+ int a2 = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+ // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the time domain [abseil-time-comparison]
+ // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
+ int a3 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
+ int a4 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+ // These should not match
+ b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToUnixSeconds(x)
+ b = 5.0 > TODOUBLE(t1);
+#undef TODOUBLE
+#define THIRTY 30.0
+ b = THIRTY > absl::ToUnixSeconds(t1);
+#undef THIRTY
+}
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -18,6 +18,7 @@
abseil-redundant-strcat-calls
abseil-str-cat-append
abseil-string-find-startswith
+ abseil-time-comparison
abseil-time-subtraction
abseil-upgrade-duration-conversions
android-cloexec-accept
Index: docs/clang-tidy/checks/abseil-time-comparison.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/abseil-time-comparison.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - abseil-time-comparison
+
+abseil-time-comparison
+======================
+
+Checks for comparisons which should be in the ``absl::Time`` domain instead
+of the integer domains.
+
+N.B.: In cases where an ``absl::Time`` is being converted to an integer,
+alignment may occur. If the comparison depends on this alingment, doing the
+comparison in the ``absl::Time`` domain may yield a different result. In
+practice this is very rare, and still indicates a bug which should be fixed.
+
+Examples:
+
+.. code-block:: c++
+
+ // Original - Comparison in the integer domain
+ int x;
+ absl::Time t;
+ if (x < absl::ToUnixSeconds(t)) ...
+
+ // Suggested - Compare in the absl::Time domain instead
+ if (absl::FromUnixSeconds(x) < t) ...
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,11 @@
Finds and fixes cases where ``absl::Duration`` values are being converted to
numeric types and back again.
+- New :doc:`abseil-time-comparison
+ <clang-tidy/checks/abseil-time-comparison>` check.
+
+ Prefer comparisons in the ``absl::Time`` domain.
+
- New :doc:`abseil-time-subtraction
<clang-tidy/checks/abseil-time-subtraction>` check.
Index: clang-tidy/abseil/TimeComparisonCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/abseil/TimeComparisonCheck.h
@@ -0,0 +1,35 @@
+//===--- TimeComparisonCheck.h - clang-tidy ---------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the `absl::Time` domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-time-comparison.html
+class TimeComparisonCheck : public ClangTidyCheck {
+public:
+ TimeComparisonCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
Index: clang-tidy/abseil/TimeComparisonCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/abseil/TimeComparisonCheck.cpp
@@ -0,0 +1,65 @@
+//===--- TimeComparisonCheck.cpp - clang-tidy --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TimeComparisonCheck.h"
+#include "DurationRewriter.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void TimeComparisonCheck::registerMatchers(MatchFinder *Finder) {
+ auto Matcher =
+ binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+ hasOperatorName("=="), hasOperatorName("<="),
+ hasOperatorName("<")),
+ hasEitherOperand(ignoringImpCasts(callExpr(
+ callee(functionDecl(TimeConversionFunction())
+ .bind("function_decl"))))))
+ .bind("binop");
+
+ Finder->addMatcher(Matcher, this);
+}
+
+void TimeComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+ llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(
+ Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+ if (!Scale)
+ return;
+
+ // In most cases, we'll only need to rewrite one of the sides, but we also
+ // want to handle the case of rewriting both sides. This is much simpler if
+ // we unconditionally try and rewrite both, and let the rewriter determine
+ // if nothing needs to be done.
+ if (!isNotInMacro(Result, Binop->getLHS()) ||
+ !isNotInMacro(Result, Binop->getRHS()))
+ return;
+
+ std::string LhsReplacement =
+ rewriteExprFromNumberToTime(Result, *Scale, Binop->getLHS());
+ std::string RhsReplacement =
+ rewriteExprFromNumberToTime(Result, *Scale, Binop->getRHS());
+
+ diag(Binop->getBeginLoc(), "perform comparison in the time domain")
+ << FixItHint::CreateReplacement(Binop->getSourceRange(),
+ (llvm::Twine(LhsReplacement) + " " +
+ Binop->getOpcodeStr() + " " +
+ RhsReplacement)
+ .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/abseil/CMakeLists.txt
===================================================================
--- clang-tidy/abseil/CMakeLists.txt
+++ clang-tidy/abseil/CMakeLists.txt
@@ -17,6 +17,7 @@
RedundantStrcatCallsCheck.cpp
StrCatAppendCheck.cpp
StringFindStartswithCheck.cpp
+ TimeComparisonCheck.cpp
TimeSubtractionCheck.cpp
UpgradeDurationConversionsCheck.cpp
Index: clang-tidy/abseil/AbseilTidyModule.cpp
===================================================================
--- clang-tidy/abseil/AbseilTidyModule.cpp
+++ clang-tidy/abseil/AbseilTidyModule.cpp
@@ -23,6 +23,7 @@
#include "RedundantStrcatCallsCheck.h"
#include "StringFindStartswithCheck.h"
#include "StrCatAppendCheck.h"
+#include "TimeComparisonCheck.h"
#include "TimeSubtractionCheck.h"
#include "UpgradeDurationConversionsCheck.h"
@@ -60,6 +61,8 @@
"abseil-str-cat-append");
CheckFactories.registerCheck<StringFindStartswithCheck>(
"abseil-string-find-startswith");
+ CheckFactories.registerCheck<TimeComparisonCheck>(
+ "abseil-time-comparison");
CheckFactories.registerCheck<TimeSubtractionCheck>(
"abseil-time-subtraction");
CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
