[clang-tools-extra] r355835 - [clang-tidy] Add the abseil-time-compare check
Hyrum Wright via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 11 09:47:45 PDT 2019
Author: hwright
Date: Mon Mar 11 09:47:45 2019
New Revision: 355835
URL: http://llvm.org/viewvc/llvm-project?rev=355835&view=rev
Log:
[clang-tidy] Add the abseil-time-compare check
This is an analog of the abseil-duration-comparison check, but for the
absl::Time domain. It has a similar implementation and tests.
Differential Revision: https://reviews.llvm.org/D58977
Added:
clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.cpp
- copied, changed from r355820, clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-time-comparison.rst
clang-tools-extra/trunk/test/clang-tidy/abseil-time-comparison.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
Modified: clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/AbseilTidyModule.cpp Mon Mar 11 09:47:45 2019
@@ -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 @@ public:
"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>(
Modified: clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/CMakeLists.txt Mon Mar 11 09:47:45 2019
@@ -17,6 +17,7 @@ add_clang_library(clangTidyAbseilModule
RedundantStrcatCallsCheck.cpp
StrCatAppendCheck.cpp
StringFindStartswithCheck.cpp
+ TimeComparisonCheck.cpp
TimeSubtractionCheck.cpp
UpgradeDurationConversionsCheck.cpp
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Mon Mar 11 09:47:45 2019
@@ -19,14 +19,10 @@ namespace tidy {
namespace abseil {
void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
- auto Matcher =
- binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
- hasOperatorName("=="), hasOperatorName("<="),
- hasOperatorName("<")),
- hasEitherOperand(ignoringImpCasts(callExpr(
- callee(functionDecl(DurationConversionFunction())
- .bind("function_decl"))))))
- .bind("binop");
+ auto Matcher = expr(comparisonOperatorWithCallee(functionDecl(
+ functionDecl(DurationConversionFunction())
+ .bind("function_decl"))))
+ .bind("binop");
Finder->addMatcher(Matcher, this);
}
Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h Mon Mar 11 09:47:45 2019
@@ -124,6 +124,16 @@ AST_MATCHER_FUNCTION(ast_matchers::inter
"::absl::ToUnixMillis", "::absl::ToUnixMicros", "::absl::ToUnixNanos"));
}
+AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher<Stmt>,
+ comparisonOperatorWithCallee,
+ ast_matchers::internal::Matcher<Decl>, funcDecl) {
+ using namespace clang::ast_matchers;
+ return binaryOperator(
+ anyOf(hasOperatorName(">"), hasOperatorName(">="), hasOperatorName("=="),
+ hasOperatorName("<="), hasOperatorName("<")),
+ hasEitherOperand(ignoringImpCasts(callExpr(callee(funcDecl)))));
+}
+
} // namespace abseil
} // namespace tidy
} // namespace clang
Copied: clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.cpp (from r355820, clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp)
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.cpp?p2=clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.cpp&p1=clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp&r1=355820&r2=355835&rev=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.cpp Mon Mar 11 09:47:45 2019
@@ -1,4 +1,5 @@
-//===--- DurationComparisonCheck.cpp - clang-tidy -------------------------===//
+//===--- 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.
@@ -6,7 +7,7 @@
//
//===----------------------------------------------------------------------===//
-#include "DurationComparisonCheck.h"
+#include "TimeComparisonCheck.h"
#include "DurationRewriter.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
@@ -18,39 +19,36 @@ namespace clang {
namespace tidy {
namespace abseil {
-void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
+void TimeComparisonCheck::registerMatchers(MatchFinder *Finder) {
auto Matcher =
- binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
- hasOperatorName("=="), hasOperatorName("<="),
- hasOperatorName("<")),
- hasEitherOperand(ignoringImpCasts(callExpr(
- callee(functionDecl(DurationConversionFunction())
- .bind("function_decl"))))))
+ expr(comparisonOperatorWithCallee(functionDecl(
+ functionDecl(TimeConversionFunction()).bind("function_decl"))))
.bind("binop");
Finder->addMatcher(Matcher, this);
}
-void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+void TimeComparisonCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
- llvm::Optional<DurationScale> Scale = getScaleForDurationInverse(
+ llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(
Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
if (!Scale)
return;
+ if (isInMacro(Result, Binop->getLHS()) || isInMacro(Result, Binop->getRHS()))
+ 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 (isInMacro(Result, Binop->getLHS()) || isInMacro(Result, Binop->getRHS()))
- return;
std::string LhsReplacement =
- rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
+ rewriteExprFromNumberToTime(Result, *Scale, Binop->getLHS());
std::string RhsReplacement =
- rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+ rewriteExprFromNumberToTime(Result, *Scale, Binop->getRHS());
- diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
+ diag(Binop->getBeginLoc(), "perform comparison in the time domain")
<< FixItHint::CreateReplacement(Binop->getSourceRange(),
(llvm::Twine(LhsReplacement) + " " +
Binop->getOpcodeStr() + " " +
Added: clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.h?rev=355835&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/abseil/TimeComparisonCheck.h Mon Mar 11 09:47:45 2019
@@ -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
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Mar 11 09:47:45 2019
@@ -85,6 +85,12 @@ Improvements to clang-tidy
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 instead of the integer
+ domain.
+
- New :doc:`abseil-time-subtraction
<clang-tidy/checks/abseil-time-subtraction>` check.
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-time-comparison.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-time-comparison.rst?rev=355835&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-time-comparison.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-time-comparison.rst Mon Mar 11 09:47:45 2019
@@ -0,0 +1,23 @@
+.. title:: clang-tidy - abseil-time-comparison
+
+abseil-time-comparison
+======================
+
+Prefer comparisons in the ``absl::Time`` domain instead of the integer domain.
+
+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) ...
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=355835&r1=355834&r2=355835&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Mon Mar 11 09:47:45 2019
@@ -18,6 +18,7 @@ Clang-Tidy Checks
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
Added: clang-tools-extra/trunk/test/clang-tidy/abseil-time-comparison.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/abseil-time-comparison.cpp?rev=355835&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/abseil-time-comparison.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/abseil-time-comparison.cpp Mon Mar 11 09:47:45 2019
@@ -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
+}
More information about the cfe-commits
mailing list