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 +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
