Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "DurationComparisonCheck.h" #include "DurationDivisionCheck.h" #include "DurationFactoryFloatCheck.h" #include "DurationFactoryScaleCheck.h" @@ -27,6 +28,8 @@ class AbseilModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "abseil-duration-comparison"); CheckFactories.registerCheck( "abseil-duration-division"); CheckFactories.registerCheck( Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -2,9 +2,11 @@ add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp + DurationComparisonCheck.cpp DurationDivisionCheck.cpp DurationFactoryFloatCheck.cpp DurationFactoryScaleCheck.cpp + DurationRewriter.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp Index: clang-tidy/abseil/DurationComparisonCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationComparisonCheck.h @@ -0,0 +1,36 @@ +//===--- DurationComparisonCheck.h - clang-tidy -----------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Prefer comparison in the absl::Duration domain instead of the numeric +// domain. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-comparison.html +class DurationComparisonCheck : public ClangTidyCheck { +public: + DurationComparisonCheck(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_DURATIONCOMPARISONCHECK_H Index: clang-tidy/abseil/DurationComparisonCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationComparisonCheck.cpp @@ -0,0 +1,175 @@ +//===--- DurationComparisonCheck.cpp - clang-tidy -------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "DurationComparisonCheck.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 { + +// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`), +// return its `DurationScale`, or `None` if a match is not found. +static llvm::Optional getScaleForInverse(llvm::StringRef Name) { + static const std::unordered_map ScaleMap( + {{"ToDoubleHours", DurationScale::Hours}, + {"ToInt64Hours", DurationScale::Hours}, + {"ToDoubleMinutes", DurationScale::Minutes}, + {"ToInt64Minutes", DurationScale::Minutes}, + {"ToDoubleSeconds", DurationScale::Seconds}, + {"ToInt64Seconds", DurationScale::Seconds}, + {"ToDoubleMilliseconds", DurationScale::Milliseconds}, + {"ToInt64Milliseconds", DurationScale::Milliseconds}, + {"ToDoubleMicroseconds", DurationScale::Microseconds}, + {"ToInt64Microseconds", DurationScale::Microseconds}, + {"ToDoubleNanoseconds", DurationScale::Nanoseconds}, + {"ToInt64Nanoseconds", DurationScale::Nanoseconds}}); + + auto ScaleIter = ScaleMap.find(std::string(Name)); + if (ScaleIter == ScaleMap.end()) + return llvm::None; + + return ScaleIter->second; +} + +// Given a `Scale` return the inverse functions for it. +static const std::pair & +getInverseForScale(DurationScale Scale) { + static const std::unordered_map> + InverseMap( + {{DurationScale::Hours, + std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}, + {DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes", + "::absl::ToInt64Minutes")}, + {DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds", + "::absl::ToInt64Seconds")}, + {DurationScale::Milliseconds, + std::make_pair("::absl::ToDoubleMilliseconds", + "::absl::ToInt64Milliseconds")}, + {DurationScale::Microseconds, + std::make_pair("::absl::ToDoubleMicroseconds", + "::absl::ToInt64Microseconds")}, + {DurationScale::Nanoseconds, + std::make_pair("::absl::ToDoubleNanoseconds", + "::absl::ToInt64Nanoseconds")}}); + + // We know our map contains all the Scale values, so we can skip the + // nonexistence check. + auto InverseIter = InverseMap.find(Scale); + assert(InverseIter != InverseMap.end() && "Unexpected scale found"); + return InverseIter->second; +} + +// If `Node` is a call to the inverse of `Scale`, return that inverse's +// argument, otherwise None. +static llvm::Optional +maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result, + DurationScale Scale, const Expr &Node) { + const std::pair &InverseFunctions = + getInverseForScale(Scale); + if (const auto *MaybeCallArg = selectFirst( + "e", match(callExpr(callee(functionDecl( + hasAnyName(InverseFunctions.first.c_str(), + InverseFunctions.second.c_str()))), + hasArgument(0, expr().bind("e"))), + Node, *Result.Context))) { + return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str(); + } + + return llvm::None; +} + +// Assuming `Node` has type `double` or `int` representing a time interval of +// `Scale`, return the expression to make it a suitable `Duration`. +static llvm::Optional rewriteExprFromNumberToDuration( + const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale, + const Expr *Node) { + const Expr &RootNode = *Node->IgnoreParenImpCasts(); + + if (RootNode.getBeginLoc().isMacroID()) + return llvm::None; + + // First check to see if we can undo a complimentary function call. + if (llvm::Optional MaybeRewrite = + maybeRewriteInverseDurationCall(Result, Scale, RootNode)) + return *MaybeRewrite; + + if (IsLiteralZero(Result, RootNode)) + return std::string("absl::ZeroDuration()"); + + return (llvm::Twine(getFactoryForScale(Scale)) + "(" + + simplifyDurationFactoryArg(Result, RootNode) + ")") + .str(); +} + +void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) { + auto Matcher = + binaryOperator( + anyOf(hasOperatorName(">"), hasOperatorName(">="), + hasOperatorName("=="), hasOperatorName("<="), + hasOperatorName("<")), + hasEitherOperand(ignoringImpCasts(callExpr(callee( + functionDecl( + hasAnyName( + "::absl::ToDoubleHours", "::absl::ToDoubleMinutes", + "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds", + "::absl::ToDoubleMicroseconds", + "::absl::ToDoubleNanoseconds", "::absl::ToInt64Hours", + "::absl::ToInt64Minutes", "::absl::ToInt64Seconds", + "::absl::ToInt64Milliseconds", + "::absl::ToInt64Microseconds", + "::absl::ToInt64Nanoseconds")) + .bind("function_decl")))))) + .bind("binop"); + + Finder->addMatcher(Matcher, this); +} + +void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Binop = Result.Nodes.getNodeAs("binop"); + + // Don't try to replace things inside of macro definitions. + if (Binop->getExprLoc().isMacroID()) + return; + + llvm::Optional Scale = getScaleForInverse( + Result.Nodes.getNodeAs("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. + llvm::Optional LhsReplacement = + rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS()); + llvm::Optional RhsReplacement = + rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS()); + + if (!(LhsReplacement && RhsReplacement)) + return; + + diag(Binop->getBeginLoc(), + "perform duration comparison in the duration domain") + << FixItHint::CreateReplacement(Binop->getSourceRange(), + (llvm::Twine(*LhsReplacement) + " " + + Binop->getOpcodeStr() + " " + + *RhsReplacement) + .str()); +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: clang-tidy/abseil/DurationDivisionCheck.h =================================================================== --- clang-tidy/abseil/DurationDivisionCheck.h +++ clang-tidy/abseil/DurationDivisionCheck.h @@ -18,7 +18,7 @@ // Find potential incorrect uses of integer division of absl::Duration objects. // -// For the user-facing documentation see: +// For the user-facing documentation see: // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html class DurationDivisionCheck : public ClangTidyCheck { Index: clang-tidy/abseil/DurationFactoryFloatCheck.cpp =================================================================== --- clang-tidy/abseil/DurationFactoryFloatCheck.cpp +++ clang-tidy/abseil/DurationFactoryFloatCheck.cpp @@ -8,6 +8,7 @@ //===----------------------------------------------------------------------===// #include "DurationFactoryFloatCheck.h" +#include "DurationRewriter.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Tooling/FixIt.h" @@ -18,19 +19,6 @@ namespace tidy { namespace abseil { -// Returns an integer if the fractional part of a `FloatingLiteral` is `0`. -static llvm::Optional -truncateIfIntegral(const FloatingLiteral &FloatLiteral) { - double Value = FloatLiteral.getValueAsApproximateDouble(); - if (std::fmod(Value, 1) == 0) { - if (Value >= static_cast(1u << 31)) - return llvm::None; - - return llvm::APSInt::get(static_cast(Value)); - } - return llvm::None; -} - // Returns `true` if `Range` is inside a macro definition. static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result, SourceRange Range) { @@ -46,17 +34,13 @@ callee(functionDecl(hasAnyName( "absl::Nanoseconds", "absl::Microseconds", "absl::Milliseconds", "absl::Seconds", "absl::Minutes", "absl::Hours"))), - hasArgument(0, - anyOf(cxxStaticCastExpr( - hasDestinationType(realFloatingPointType()), - hasSourceExpression(expr().bind("cast_arg"))), - cStyleCastExpr( - hasDestinationType(realFloatingPointType()), - hasSourceExpression(expr().bind("cast_arg"))), - cxxFunctionalCastExpr( - hasDestinationType(realFloatingPointType()), - hasSourceExpression(expr().bind("cast_arg"))), - floatLiteral().bind("float_literal")))) + hasArgument(0, anyOf(cxxStaticCastExpr( + hasDestinationType(realFloatingPointType())), + cStyleCastExpr( + hasDestinationType(realFloatingPointType())), + cxxFunctionalCastExpr( + hasDestinationType(realFloatingPointType())), + floatLiteral()))) .bind("call"), this); } @@ -73,31 +57,16 @@ if (Arg->getBeginLoc().isMacroID()) return; - // Check for casts to `float` or `double`. - if (const auto *MaybeCastArg = Result.Nodes.getNodeAs("cast_arg")) { + llvm::Optional SimpleArg = stripFloatCast(Result, *Arg); + if (!SimpleArg) + SimpleArg = stripFloatLiteralFraction(Result, *Arg); + + if (SimpleArg) { diag(MatchedCall->getBeginLoc(), (llvm::Twine("use the integer version of absl::") + MatchedCall->getDirectCallee()->getName()) .str()) - << FixItHint::CreateReplacement( - Arg->getSourceRange(), - tooling::fixit::getText(*MaybeCastArg, *Result.Context)); - return; - } - - // Check for floats without fractional components. - if (const auto *LitFloat = - Result.Nodes.getNodeAs("float_literal")) { - // Attempt to simplify a `Duration` factory call with a literal argument. - if (llvm::Optional IntValue = truncateIfIntegral(*LitFloat)) { - diag(MatchedCall->getBeginLoc(), - (llvm::Twine("use the integer version of absl::") + - MatchedCall->getDirectCallee()->getName()) - .str()) - << FixItHint::CreateReplacement(LitFloat->getSourceRange(), - IntValue->toString(/*radix=*/10)); - return; - } + << FixItHint::CreateReplacement(Arg->getSourceRange(), *SimpleArg); } } Index: clang-tidy/abseil/DurationFactoryScaleCheck.cpp =================================================================== --- clang-tidy/abseil/DurationFactoryScaleCheck.cpp +++ clang-tidy/abseil/DurationFactoryScaleCheck.cpp @@ -8,6 +8,7 @@ //===----------------------------------------------------------------------===// #include "DurationFactoryScaleCheck.h" +#include "DurationRewriter.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Tooling/FixIt.h" @@ -18,20 +19,6 @@ namespace tidy { namespace abseil { -namespace { - -// Potential scales of our inputs. -enum class DurationScale { - Hours, - Minutes, - Seconds, - Milliseconds, - Microseconds, - Nanoseconds, -}; - -} // namespace - // Given the name of a duration factory function, return the appropriate // `DurationScale` for that factory. If no factory can be found for // `FactoryName`, return `None`. @@ -129,26 +116,6 @@ return llvm::None; } -// Given a `Scale`, return the appropriate factory function call for -// constructing a `Duration` for that scale. -static llvm::StringRef GetFactoryForScale(DurationScale Scale) { - switch (Scale) { - case DurationScale::Hours: - return "absl::Hours"; - case DurationScale::Minutes: - return "absl::Minutes"; - case DurationScale::Seconds: - return "absl::Seconds"; - case DurationScale::Milliseconds: - return "absl::Milliseconds"; - case DurationScale::Microseconds: - return "absl::Microseconds"; - case DurationScale::Nanoseconds: - return "absl::Nanoseconds"; - } - llvm_unreachable("unknown scaling factor"); -} - void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( callExpr( @@ -160,8 +127,7 @@ hasArgument( 0, ignoringImpCasts(anyOf( - integerLiteral(equals(0)).bind("zero"), - floatLiteral(equals(0.0)).bind("zero"), + integerLiteral(equals(0)), floatLiteral(equals(0.0)), binaryOperator(hasOperatorName("*"), hasEitherOperand(ignoringImpCasts( anyOf(integerLiteral(), floatLiteral())))) @@ -185,7 +151,7 @@ return; // We first handle the cases of literal zero (both float and integer). - if (Result.Nodes.getNodeAs("zero")) { + if (IsLiteralZero(Result, *Arg)) { diag(Call->getBeginLoc(), "use ZeroDuration() for zero-length time intervals") << FixItHint::CreateReplacement(Call->getSourceRange(), @@ -244,7 +210,7 @@ diag(Call->getBeginLoc(), "internal duration scaling can be removed") << FixItHint::CreateReplacement( Call->getSourceRange(), - (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" + + (llvm::Twine(getFactoryForScale(*NewScale)) + "(" + tooling::fixit::getText(*Remainder, *Result.Context) + ")") .str()); } @@ -257,7 +223,7 @@ diag(Call->getBeginLoc(), "internal duration scaling can be removed") << FixItHint::CreateReplacement( Call->getSourceRange(), - (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" + + (llvm::Twine(getFactoryForScale(*NewScale)) + "(" + tooling::fixit::getText(*Remainder, *Result.Context) + ")") .str()); } Index: clang-tidy/abseil/DurationRewriter.h =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationRewriter.h @@ -0,0 +1,64 @@ +//===--- DurationRewriter.h - clang-tidy ------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H + +#include "clang/ASTMatchers/ASTMatchFinder.h" + +namespace clang { +namespace tidy { +namespace abseil { + +// Duration factory and conversion scales +enum class DurationScale { + Hours, + Minutes, + Seconds, + Milliseconds, + Microseconds, + Nanoseconds, +}; + +// Given a `Scale`, return the appropriate factory function call for +// constructing a `Duration` for that scale. +llvm::StringRef getFactoryForScale(DurationScale Scale); + +// Determine if `Node` represents a literal floating point or integral zero. +bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr &Node); + +// Possibly strip a floating point cast expression. +// +// If `Node` represents an explicit cast to a floating point type, return +// the textual context of the cast argument, otherwise `None`. +llvm::Optional +stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr &Node); + +// Possibly remove the fractional part of a floating point literal. +// +// If `Node` represents a floating point literal with a zero fractional part, +// return the textual context of the integral part, otherwise `None`. +llvm::Optional +stripFloatLiteralFraction(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr &Node); + +// Possibly further simplify a duration factory function's argument, without +// changing the scale of the factory function. Return that simplification or +// the text of the argument if no simplification is possible. +std::string +simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr &Node); + +} // namespace abseil +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H Index: clang-tidy/abseil/DurationRewriter.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationRewriter.cpp @@ -0,0 +1,110 @@ +//===--- DurationRewriter.cpp - clang-tidy --------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "DurationRewriter.h" +#include "clang/Tooling/FixIt.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace abseil { + +// Returns an integer if the fractional part of a `FloatingLiteral` is `0`. +static llvm::Optional +truncateIfIntegral(const FloatingLiteral &FloatLiteral) { + double Value = FloatLiteral.getValueAsApproximateDouble(); + if (std::fmod(Value, 1) == 0) { + if (Value >= static_cast(1u << 31)) + return llvm::None; + + return llvm::APSInt::get(static_cast(Value)); + } + return llvm::None; +} + +llvm::StringRef getFactoryForScale(DurationScale Scale) { + switch (Scale) { + case DurationScale::Hours: + return "absl::Hours"; + case DurationScale::Minutes: + return "absl::Minutes"; + case DurationScale::Seconds: + return "absl::Seconds"; + case DurationScale::Milliseconds: + return "absl::Milliseconds"; + case DurationScale::Microseconds: + return "absl::Microseconds"; + case DurationScale::Nanoseconds: + return "absl::Nanoseconds"; + } + llvm_unreachable("unknown scaling factor"); +} + +bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) { + return selectFirst( + "val", + match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)), + floatLiteral(equals(0.0))))) + .bind("val"), + Node, *Result.Context)) != nullptr; +} + +llvm::Optional +stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result, + const Expr &Node) { + if (const auto *MaybeCastArg = selectFirst( + "cast_arg", + match(expr(anyOf(cxxStaticCastExpr( + hasDestinationType(realFloatingPointType()), + hasSourceExpression(expr().bind("cast_arg"))), + cStyleCastExpr( + hasDestinationType(realFloatingPointType()), + hasSourceExpression(expr().bind("cast_arg"))), + cxxFunctionalCastExpr( + hasDestinationType(realFloatingPointType()), + hasSourceExpression(expr().bind("cast_arg"))))), + Node, *Result.Context))) { + return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str(); + } + + return llvm::None; +} + +llvm::Optional +stripFloatLiteralFraction(const MatchFinder::MatchResult &Result, + const Expr &Node) { + if (const auto *LitFloat = llvm::dyn_cast(&Node)) { + // Attempt to simplify a `Duration` factory call with a literal argument. + if (llvm::Optional IntValue = truncateIfIntegral(*LitFloat)) { + return IntValue->toString(/*radix=*/10); + } + } + + return llvm::None; +} + +std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result, + const Expr &Node) { + // Check for an explicit cast to `float` or `double`. + if (llvm::Optional MaybeArg = stripFloatCast(Result, Node)) + return *MaybeArg; + + // Check for floats without fractional components. + if (llvm::Optional MaybeArg = + stripFloatLiteralFraction(Result, Node)) + return *MaybeArg; + + // We couldn't simplify any further, so return the argument text. + return tooling::fixit::getText(Node, *Result.Context).str(); +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -67,6 +67,12 @@ Improvements to clang-tidy -------------------------- +- New :doc:`abseil-duration-comparison + ` check. + + Checks for comparisons which should be done in the ``absl::Duration`` domain + instead of the float of integer domains. + - New :doc:`abseil-duration-division ` check. Index: docs/clang-tidy/checks/abseil-duration-comparison.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-duration-comparison.rst @@ -0,0 +1,33 @@ +.. title:: clang-tidy - abseil-duration-comparison + +abseil-duration-comparison +========================== + +Checks for comparisons which should be in the ``absl::Duration`` domain instead +of the floating point or integer domains. + +N.B.: In cases where a ``Duration`` was being converted to an integer and then +compared against a floating-point value, truncation during the ``Duration`` +conversion might 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 floating point domain + double x; + absl::Duration d; + if (x < absl::ToDoubleSeconds(d)) ... + + // Suggested - Compare in the absl::Duration domain instead + if (absl::Seconds(x) < d) ... + + + // Original - Comparison in the integer domain + int x; + absl::Duration d; + if (x < absl::ToInt64Microseconds(d)) ... + + // Suggested - Compare in the absl::Duration domain instead + if (absl::Microseconds(x) < d) ... Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -4,6 +4,7 @@ ================= .. toctree:: + abseil-duration-comparison abseil-duration-division abseil-duration-factory-float abseil-duration-factory-scale Index: test/clang-tidy/abseil-duration-comparison.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-duration-comparison.cpp @@ -0,0 +1,189 @@ +// RUN: %check_clang_tidy %s abseil-duration-comparison %t + +// Mimic the implementation of absl::Duration +namespace absl { + +class Duration {}; +class Time{}; + +Duration Nanoseconds(long long); +Duration Microseconds(long long); +Duration Milliseconds(long long); +Duration Seconds(long long); +Duration Minutes(long long); +Duration Hours(long long); + +#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \ + Duration NAME(float n); \ + Duration NAME(double n); \ + template \ + Duration NAME(T n); + +GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds); +GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds); +GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds); +GENERATE_DURATION_FACTORY_OVERLOADS(Seconds); +GENERATE_DURATION_FACTORY_OVERLOADS(Minutes); +GENERATE_DURATION_FACTORY_OVERLOADS(Hours); +#undef GENERATE_DURATION_FACTORY_OVERLOADS + +using int64_t = long long int; + +double ToDoubleHours(Duration d); +double ToDoubleMinutes(Duration d); +double ToDoubleSeconds(Duration d); +double ToDoubleMilliseconds(Duration d); +double ToDoubleMicroseconds(Duration d); +double ToDoubleNanoseconds(Duration d); +int64_t ToInt64Hours(Duration d); +int64_t ToInt64Minutes(Duration d); +int64_t ToInt64Seconds(Duration d); +int64_t ToInt64Milliseconds(Duration d); +int64_t ToInt64Microseconds(Duration d); +int64_t ToInt64Nanoseconds(Duration d); + +// Relational Operators +constexpr bool operator<(Duration lhs, Duration rhs); +constexpr bool operator>(Duration lhs, Duration rhs); +constexpr bool operator>=(Duration lhs, Duration rhs); +constexpr bool operator<=(Duration lhs, Duration rhs); +constexpr bool operator==(Duration lhs, Duration rhs); +constexpr bool operator!=(Duration lhs, Duration rhs); + +// Additive Operators +inline Time operator+(Time lhs, Duration rhs); +inline Time operator+(Duration lhs, Time rhs); +inline Time operator-(Time lhs, Duration rhs); +inline Duration operator-(Time lhs, Time rhs); + +} // namespace absl + +void f() { + double x; + absl::Duration d1, d2; + bool b; + absl::Time t1, t2; + + // Check against the RHS + b = x > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) > d1; + b = x >= absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) >= d1; + b = x == absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) == d1; + b = x <= absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) <= d1; + b = x < absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) < d1; + b = x == absl::ToDoubleSeconds(t1 - t2); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) == t1 - t2; + b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 > d2; + + // Check against the LHS + b = absl::ToDoubleSeconds(d1) < x; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 < absl::Seconds(x); + b = absl::ToDoubleSeconds(d1) <= x; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 <= absl::Seconds(x); + b = absl::ToDoubleSeconds(d1) == x; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 == absl::Seconds(x); + b = absl::ToDoubleSeconds(d1) >= x; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 >= absl::Seconds(x); + b = absl::ToDoubleSeconds(d1) > x; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 > absl::Seconds(x); + + // Comparison against zero + b = absl::ToDoubleSeconds(d1) < 0.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 < absl::ZeroDuration(); + b = absl::ToDoubleSeconds(d1) < 0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: d1 < absl::ZeroDuration(); + + // Scales other than Seconds + b = x > absl::ToDoubleMicroseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Microseconds(x) > d1; + b = x >= absl::ToDoubleMilliseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Milliseconds(x) >= d1; + b = x == absl::ToDoubleNanoseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Nanoseconds(x) == d1; + b = x <= absl::ToDoubleMinutes(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Minutes(x) <= d1; + b = x < absl::ToDoubleHours(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Hours(x) < d1; + + // Integer comparisons + b = x > absl::ToInt64Microseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Microseconds(x) > d1; + b = x >= absl::ToInt64Milliseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Milliseconds(x) >= d1; + b = x == absl::ToInt64Nanoseconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Nanoseconds(x) == d1; + b = x == absl::ToInt64Seconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(x) == d1; + b = x <= absl::ToInt64Minutes(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Minutes(x) <= d1; + b = x < absl::ToInt64Hours(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Hours(x) < d1; + + // Other abseil-duration checks folded into this one + b = static_cast(5) > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(5) > d1; + b = double(5) > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(5) > d1; + b = float(5) > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(5) > d1; + b = ((double)5) > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(5) > d1; + b = 5.0 > absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: absl::Seconds(5) > d1; + + // A long expression + bool some_condition; + int very_very_very_very_long_variable_name; + absl::Duration SomeDuration; + if (some_condition && very_very_very_very_long_variable_name + < absl::ToDoubleSeconds(SomeDuration)) { + // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform duration comparison in the duration domain [abseil-duration-comparison] + // CHECK-FIXES: if (some_condition && absl::Seconds(very_very_very_very_long_variable_name) < SomeDuration) { + return; + } + + // These should not match + b = 6 < 4; + +#define TODOUBLE(x) absl::ToDoubleSeconds(x) + b = 5.0 > TODOUBLE(d1); +#undef TODOUBLE +#define THIRTY 30.0 + b = THIRTY > absl::ToDoubleSeconds(d1); +#undef THIRTY +}