From 1603447b037968f9ea6e969fec2831f74a239d25 Mon Sep 17 00:00:00 2001 From: Hyrum Wright Date: Mon, 11 Mar 2019 16:47:45 +0000 Subject: [PATCH] [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 llvm-svn: 355835 --- .../clang-tidy/abseil/AbseilTidyModule.cpp | 3 + clang-tools-extra/clang-tidy/abseil/CMakeLists.txt | 1 + .../clang-tidy/abseil/DurationComparisonCheck.cpp | 12 +- .../clang-tidy/abseil/DurationRewriter.h | 10 ++ .../clang-tidy/abseil/TimeComparisonCheck.cpp | 61 ++++++++++ .../clang-tidy/abseil/TimeComparisonCheck.h | 35 ++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/abseil-time-comparison.rst | 23 ++++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../test/clang-tidy/abseil-time-comparison.cpp | 129 +++++++++++++++++++++ 10 files changed, 273 insertions(+), 8 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/abseil-time-comparison.rst create mode 100644 clang-tools-extra/test/clang-tidy/abseil-time-comparison.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index 514b7f7..c70ef90 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/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 @@ public: "abseil-str-cat-append"); CheckFactories.registerCheck( "abseil-string-find-startswith"); + CheckFactories.registerCheck( + "abseil-time-comparison"); CheckFactories.registerCheck( "abseil-time-subtraction"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index f061603..3f88da6 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -17,6 +17,7 @@ add_clang_library(clangTidyAbseilModule RedundantStrcatCallsCheck.cpp StrCatAppendCheck.cpp StringFindStartswithCheck.cpp + TimeComparisonCheck.cpp TimeSubtractionCheck.cpp UpgradeDurationConversionsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp index 3b19e5f..a6e12dd 100644 --- a/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp @@ -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); } diff --git a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h index 32f8167..1ae312c 100644 --- a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h +++ b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h @@ -124,6 +124,16 @@ AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, "::absl::ToUnixMillis", "::absl::ToUnixMicros", "::absl::ToUnixNanos")); } +AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher, + comparisonOperatorWithCallee, + ast_matchers::internal::Matcher, 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 diff --git a/clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.cpp b/clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.cpp new file mode 100644 index 0000000..391f38f --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.cpp @@ -0,0 +1,61 @@ +//===--- 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 = + expr(comparisonOperatorWithCallee(functionDecl( + functionDecl(TimeConversionFunction()).bind("function_decl")))) + .bind("binop"); + + Finder->addMatcher(Matcher, this); +} + +void TimeComparisonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Binop = Result.Nodes.getNodeAs("binop"); + + llvm::Optional Scale = getScaleForTimeInverse( + Result.Nodes.getNodeAs("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. + 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 diff --git a/clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.h b/clang-tools-extra/clang-tidy/abseil/TimeComparisonCheck.h new file mode 100644 index 0000000..5598433 --- /dev/null +++ b/clang-tools-extra/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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3d3edef..9f464f9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -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 + ` check. + + Prefer comparisons in the ``absl::Time`` domain instead of the integer + domain. + - New :doc:`abseil-time-subtraction ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-time-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-time-comparison.rst new file mode 100644 index 0000000..130459b --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-time-comparison.rst @@ -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) ... diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 353ea37..80aba4a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -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 diff --git a/clang-tools-extra/test/clang-tidy/abseil-time-comparison.cpp b/clang-tools-extra/test/clang-tidy/abseil-time-comparison.cpp new file mode 100644 index 0000000..ab03020 --- /dev/null +++ b/clang-tools-extra/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 +} -- 2.7.4