Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -14,6 +14,7 @@ #include "DurationDivisionCheck.h" #include "DurationFactoryFloatCheck.h" #include "DurationFactoryScaleCheck.h" +#include "DurationSubtractionCheck.h" #include "FasterStrsplitDelimiterCheck.h" #include "NoInternalDependenciesCheck.h" #include "NoNamespaceCheck.h" @@ -37,6 +38,8 @@ "abseil-duration-factory-float"); CheckFactories.registerCheck( "abseil-duration-factory-scale"); + CheckFactories.registerCheck( + "abseil-duration-subtraction"); CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck( Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -7,6 +7,7 @@ DurationFactoryFloatCheck.cpp DurationFactoryScaleCheck.cpp DurationRewriter.cpp + DurationSubtractionCheck.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp Index: clang-tidy/abseil/DurationComparisonCheck.cpp =================================================================== --- clang-tidy/abseil/DurationComparisonCheck.cpp +++ clang-tidy/abseil/DurationComparisonCheck.cpp @@ -19,101 +19,6 @@ 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 llvm::DenseMap 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 Expr *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(">="), Index: clang-tidy/abseil/DurationRewriter.h =================================================================== --- clang-tidy/abseil/DurationRewriter.h +++ clang-tidy/abseil/DurationRewriter.h @@ -19,8 +19,8 @@ namespace abseil { /// Duration factory and conversion scales -enum class DurationScale : std::int8_t { - Hours, +enum class DurationScale : std::uint8_t { + Hours = 0, Minutes, Seconds, Milliseconds, @@ -78,6 +78,16 @@ simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result, const Expr &Node); +/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`), +/// return its `DurationScale`, or `None` if a match is not found. +llvm::Optional getScaleForInverse(llvm::StringRef Name); + +/// Assuming `Node` has type `double` or `int` representing a time interval of +/// `Scale`, return the expression to make it a suitable `Duration`. +llvm::Optional rewriteExprFromNumberToDuration( + const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale, + const Expr *Node); + AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, DurationConversionFunction) { using namespace clang::ast_matchers; Index: clang-tidy/abseil/DurationRewriter.cpp =================================================================== --- clang-tidy/abseil/DurationRewriter.cpp +++ clang-tidy/abseil/DurationRewriter.cpp @@ -9,6 +9,7 @@ #include "DurationRewriter.h" #include "clang/Tooling/FixIt.h" +#include "llvm/ADT/IndexedMap.h" using namespace clang::ast_matchers; @@ -16,6 +17,13 @@ namespace tidy { namespace abseil { +struct DurationScale2IndexFunctor { + using argument_type = DurationScale; + unsigned operator()(DurationScale Scale) const { + return static_cast(Scale); + } +}; + /// Returns an integer if the fractional part of a `FloatingLiteral` is `0`. static llvm::Optional truncateIfIntegral(const FloatingLiteral &FloatLiteral) { @@ -29,6 +37,55 @@ return llvm::None; } +/// Given a `Scale` return the inverse functions for it. +static const std::pair & +getInverseForScale(DurationScale Scale) { + static const llvm::IndexedMap, + DurationScale2IndexFunctor> + InverseMap = []() { + // TODO: Revisit the immediately invoked lamba technique when + // IndexedMap gets an initializer list constructor. + llvm::IndexedMap, + DurationScale2IndexFunctor> + InverseMap; + InverseMap.resize(6); + InverseMap[DurationScale::Hours] = + std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours"); + InverseMap[DurationScale::Minutes] = + std::make_pair("::absl::ToDoubleMinutes", "::absl::ToInt64Minutes"); + InverseMap[DurationScale::Seconds] = + std::make_pair("::absl::ToDoubleSeconds", "::absl::ToInt64Seconds"); + InverseMap[DurationScale::Milliseconds] = std::make_pair( + "::absl::ToDoubleMilliseconds", "::absl::ToInt64Milliseconds"); + InverseMap[DurationScale::Microseconds] = std::make_pair( + "::absl::ToDoubleMicroseconds", "::absl::ToInt64Microseconds"); + InverseMap[DurationScale::Nanoseconds] = std::make_pair( + "::absl::ToDoubleNanoseconds", "::absl::ToInt64Nanoseconds"); + return InverseMap; + }(); + + return InverseMap[Scale]; +} + +/// If `Node` is a call to the inverse of `Scale`, return that inverse's +/// argument, otherwise None. +static llvm::Optional +rewriteInverseDurationCall(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, InverseFunctions.second))), + hasArgument(0, expr().bind("e"))), + Node, *Result.Context))) { + return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str(); + } + + return llvm::None; +} + /// Returns the factory function name for a given `Scale`. llvm::StringRef getFactoryForScale(DurationScale Scale) { switch (Scale) { @@ -104,6 +161,49 @@ return tooling::fixit::getText(Node, *Result.Context).str(); } +llvm::Optional getScaleForInverse(llvm::StringRef Name) { + static const llvm::StringMap 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; +} + +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 = + rewriteInverseDurationCall(Result, Scale, RootNode)) + return *MaybeRewrite; + + if (IsLiteralZero(Result, RootNode)) + return std::string("absl::ZeroDuration()"); + + return (llvm::Twine(getFactoryForScale(Scale)) + "(" + + simplifyDurationFactoryArg(Result, RootNode) + ")") + .str(); +} + } // namespace abseil } // namespace tidy } // namespace clang Index: clang-tidy/abseil/DurationSubtractionCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationSubtractionCheck.h @@ -0,0 +1,36 @@ +//===--- DurationSubtractionCheck.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_DURATIONSUBTRACTIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONSUBTRACTIONCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Checks for cases where subtraction should be performed in the +/// `absl::Duration` domain. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-subtraction.html +class DurationSubtractionCheck : public ClangTidyCheck { +public: + DurationSubtractionCheck(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_DURATIONSUBTRACTIONCHECK_H Index: clang-tidy/abseil/DurationSubtractionCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationSubtractionCheck.cpp @@ -0,0 +1,63 @@ +//===--- DurationSubtractionCheck.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 "DurationSubtractionCheck.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 DurationSubtractionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + binaryOperator( + hasOperatorName("-"), + hasLHS(callExpr(callee(functionDecl(DurationConversionFunction()) + .bind("function_decl")), + hasArgument(0, expr().bind("lhs_arg"))))) + .bind("binop"), + this); +} + +void DurationSubtractionCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Binop = Result.Nodes.getNodeAs("binop"); + const auto *FuncDecl = Result.Nodes.getNodeAs("function_decl"); + + // Don't try to replace things inside of macro definitions. + if (Binop->getExprLoc().isMacroID() || Binop->getExprLoc().isInvalid()) + return; + + llvm::Optional Scale = getScaleForInverse(FuncDecl->getName()); + if (!Scale) + return; + + llvm::Optional RhsReplacement = + rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS()); + if (!RhsReplacement) + return; + + const Expr *LhsArg = Result.Nodes.getNodeAs("lhs_arg"); + + diag(Binop->getBeginLoc(), "perform subtraction in the duration domain") + << FixItHint::CreateReplacement( + Binop->getSourceRange(), + (llvm::Twine("absl::") + FuncDecl->getName() + "(" + + tooling::fixit::getText(*LhsArg, *Result.Context) + " - " + + *RhsReplacement + ")") + .str()); +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -93,6 +93,12 @@ Checks for cases where arguments to ``absl::Duration`` factory functions are scaled internally and could be changed to a different factory function. +- New :doc:`abseil-duration-subtraction + ` check. + + Checks for cases where subtraction should be performed in the + ``absl::Duration`` domain. + - New :doc:`abseil-faster-strsplit-delimiter ` check. Index: docs/clang-tidy/checks/abseil-duration-subtraction.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-duration-subtraction.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - abseil-duration-subtraction + +abseil-duration-subtraction +=========================== + +Checks for cases where subtraction should be performed in the +``absl::Duration`` domain. When subtracting two values, and the first one is +known to be a conversion from ``absl::Duration``, we can infer that the second +should also be interpreted as an ``absl::Duration``, and make that inference +explicit. + +Examples: + +.. code-block:: c++ + + // Original - Subtraction in the double domain + double x; + absl::Duration d; + double result = absl::ToDoubleSeconds(d) - x; + + // Suggestion - Subtraction in the absl::Duration domain instead + double result = absl::ToDoubleSeconds(d - absl::Seconds(x)); + + + // Original - Subtraction of two Durations in the double domain + absl::Duration d1, d2; + double result = absl::ToDoubleSeconds(d1) - absl::ToDoubleSeconds(d2); + + // Suggestion - Subtraction in the absl::Duration domain instead + double result = absl::ToDoubleSeconds(d1 - d2); + +Note: As with other ``clang-tidy`` checks, it is possible that multiple fixes +may overlap (as in the case of nested expressions), so not all occurences can +be transformed in one run. Running ``clang-tidy`` multiple times will fix these +overlaps. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -8,6 +8,7 @@ abseil-duration-division abseil-duration-factory-float abseil-duration-factory-scale + abseil-duration-subtraction abseil-faster-strsplit-delimiter abseil-no-internal-dependencies abseil-no-namespace Index: test/clang-tidy/abseil-duration-subtraction.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-duration-subtraction.cpp @@ -0,0 +1,64 @@ +// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs + +#include "absl/time/time.h" + +void f() { + double x; + absl::Duration d, d1, d2; + + x = absl::ToDoubleSeconds(d) - 1.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1)) + x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(d - d1); + x = absl::ToDoubleSeconds(d) - 6.5 - 8.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0; + x = absl::ToDoubleHours(d) - 1.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1)) + x = absl::ToDoubleMinutes(d) - 1; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1)) + x = absl::ToDoubleMilliseconds(d) - 9; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9)) + x = absl::ToDoubleMicroseconds(d) - 9; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9)) + x = absl::ToDoubleNanoseconds(d) - 42; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42)) + + // We can rewrite the argument of the duration conversion +#define THIRTY absl::Seconds(30) + x = absl::ToDoubleSeconds(THIRTY) - 1.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1)) +#undef THIRTY + + // Some other contexts + if (absl::ToDoubleSeconds(d) - 1.0 > 10) {} + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {} + + // A nested occurance + x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(5)) + x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::Seconds(absl::ToDoubleSeconds(d1))); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction] + // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(absl::ToDoubleSeconds(d1))) + + // These should not match + x = 5 - 6; + x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0; + x = absl::ToDoubleSeconds(d) + 1.0; + x = absl::ToDoubleSeconds(d) * 1.0; + x = absl::ToDoubleSeconds(d) / 1.0; + +#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5 + x = MINUS_FIVE(d); +#undef MINUS_FIVE +}