From 722191be259a6d9e1426325670741a8043e9fd7e Mon Sep 17 00:00:00 2001 From: Hyrum Wright Date: Mon, 28 Jan 2019 14:03:09 +0000 Subject: [PATCH] [clang-tidy] Add the abseil-duration-addition check Differential Revision: https://reviews.llvm.org/D57185 llvm-svn: 352362 --- .../clang-tidy/abseil/AbseilTidyModule.cpp | 3 + clang-tools-extra/clang-tidy/abseil/CMakeLists.txt | 1 + .../clang-tidy/abseil/DurationAdditionCheck.cpp | 73 ++++++++++++++++ .../clang-tidy/abseil/DurationAdditionCheck.h | 35 ++++++++ .../clang-tidy/abseil/DurationRewriter.cpp | 35 ++++++++ .../clang-tidy/abseil/DurationRewriter.h | 15 ++++ clang-tools-extra/docs/ReleaseNotes.rst | 7 +- .../clang-tidy/checks/abseil-duration-addition.rst | 21 +++++ clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../test/clang-tidy/Inputs/absl/time/time.h | 13 +++ .../test/clang-tidy/abseil-duration-addition.cpp | 98 ++++++++++++++++++++++ 11 files changed, 301 insertions(+), 1 deletion(-) create mode 100644 clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/abseil-duration-addition.rst create mode 100644 clang-tools-extra/test/clang-tidy/abseil-duration-addition.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index 6c92166..b05318b 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "DurationAdditionCheck.h" #include "DurationComparisonCheck.h" #include "DurationConversionCastCheck.h" #include "DurationDivisionCheck.h" @@ -30,6 +31,8 @@ namespace abseil { class AbseilModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "abseil-duration-addition"); CheckFactories.registerCheck( "abseil-duration-comparison"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index 6a37bab..68247ba 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp + DurationAdditionCheck.cpp DurationComparisonCheck.cpp DurationConversionCastCheck.cpp DurationDivisionCheck.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.cpp new file mode 100644 index 0000000..1ff8452 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.cpp @@ -0,0 +1,73 @@ +//===--- DurationAdditionCheck.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 "DurationAdditionCheck.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 DurationAdditionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + binaryOperator(hasOperatorName("+"), + hasEitherOperand(expr(ignoringParenImpCasts( + callExpr(callee(functionDecl(TimeFactoryFunction()) + .bind("function_decl"))) + .bind("call"))))) + .bind("binop"), + this); +} + +void DurationAdditionCheck::check(const MatchFinder::MatchResult &Result) { + const BinaryOperator *Binop = + Result.Nodes.getNodeAs("binop"); + const CallExpr *Call = Result.Nodes.getNodeAs("call"); + + // Don't try to replace things inside of macro definitions. + if (Binop->getExprLoc().isMacroID() || Binop->getExprLoc().isInvalid()) + return; + + llvm::Optional Scale = getScaleForTimeInverse( + Result.Nodes.getNodeAs("function_decl")->getName()); + if (!Scale) + return; + + llvm::StringRef TimeFactory = getTimeFactoryForScale(*Scale); + + FixItHint Hint; + if (Call == Binop->getLHS()->IgnoreParenImpCasts()) { + Hint = FixItHint::CreateReplacement( + Binop->getSourceRange(), + (llvm::Twine(TimeFactory) + "(" + + tooling::fixit::getText(*Call->getArg(0), *Result.Context) + " + " + + rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS()) + ")") + .str()); + } else { + assert(Call == Binop->getRHS()->IgnoreParenImpCast() && + "Call should be found on the RHS"); + Hint = FixItHint::CreateReplacement( + Binop->getSourceRange(), + (llvm::Twine(TimeFactory) + "(" + + rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS()) + + " + " + tooling::fixit::getText(*Call->getArg(0), *Result.Context) + + ")") + .str()); + } + + diag(Binop->getBeginLoc(), "perform addition in the duration domain") << Hint; +} + +} // namespace abseil +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.h b/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.h new file mode 100644 index 0000000..64b4d89 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.h @@ -0,0 +1,35 @@ +//===--- DurationAdditionCheck.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_TIMEADDITIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMEADDITIONCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Checks for cases where addition should be performed in the +/// ``absl::Time`` domain. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-addition.html +class DurationAdditionCheck : public ClangTidyCheck { +public: + DurationAdditionCheck(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_TIMEADDITIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp index 5a3f98b..fb974f6 100644 --- a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp +++ b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp @@ -103,6 +103,25 @@ llvm::StringRef getDurationFactoryForScale(DurationScale Scale) { llvm_unreachable("unknown scaling factor"); } +/// Returns the Time factory function name for a given `Scale`. +llvm::StringRef getTimeFactoryForScale(DurationScale scale) { + switch (scale) { + case DurationScale::Hours: + return "absl::ToUnixHours"; + case DurationScale::Minutes: + return "absl::ToUnixMinutes"; + case DurationScale::Seconds: + return "absl::ToUnixSeconds"; + case DurationScale::Milliseconds: + return "absl::ToUnixMillis"; + case DurationScale::Microseconds: + return "absl::ToUnixMicros"; + case DurationScale::Nanoseconds: + return "absl::ToUnixNanos"; + } + llvm_unreachable("unknown scaling factor"); +} + /// Returns `true` if `Node` is a value which evaluates to a literal `0`. bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) { auto ZeroMatcher = @@ -197,6 +216,22 @@ llvm::Optional getScaleForDurationInverse(llvm::StringRef Name) { return ScaleIter->second; } +llvm::Optional getScaleForTimeInverse(llvm::StringRef Name) { + static const llvm::StringMap ScaleMap( + {{"ToUnixHours", DurationScale::Hours}, + {"ToUnixMinutes", DurationScale::Minutes}, + {"ToUnixSeconds", DurationScale::Seconds}, + {"ToUnixMillis", DurationScale::Milliseconds}, + {"ToUnixMicros", DurationScale::Microseconds}, + {"ToUnixNanos", DurationScale::Nanoseconds}}); + + auto ScaleIter = ScaleMap.find(std::string(Name)); + if (ScaleIter == ScaleMap.end()) + return llvm::None; + + return ScaleIter->second; +} + std::string rewriteExprFromNumberToDuration( const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale, const Expr *Node) { diff --git a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h index 1542b4c..36dd539 100644 --- a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h +++ b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h @@ -31,6 +31,9 @@ enum class DurationScale : std::uint8_t { /// constructing a `Duration` for that scale. llvm::StringRef getDurationFactoryForScale(DurationScale Scale); +/// Returns the Time factory function name for a given `Scale`. +llvm::StringRef getTimeFactoryForScale(DurationScale scale); + // Determine if `Node` represents a literal floating point or integral zero. bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result, const Expr &Node); @@ -62,6 +65,10 @@ simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result, /// return its `DurationScale`, or `None` if a match is not found. llvm::Optional getScaleForDurationInverse(llvm::StringRef Name); +/// Given the name of an inverse Time function (e.g., `ToUnixSeconds`), +/// return its `DurationScale`, or `None` if a match is not found. +llvm::Optional getScaleForTimeInverse(llvm::StringRef Name); + /// Given a `Scale` return the fully qualified inverse functions for it. /// The first returned value is the inverse for `double`, and the second /// returned value is the inverse for `int64`. @@ -99,6 +106,14 @@ AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, "::absl::Minutes", "::absl::Hours")); } +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, + TimeFactoryFunction) { + using namespace clang::ast_matchers; + return functionDecl(hasAnyName( + "::absl::ToUnixHours", "::absl::ToUnixMinutes", "::absl::ToUnixSeconds", + "::absl::ToUnixMillis", "::absl::ToUnixMicros", "::absl::ToUnixNanos")); +} + } // namespace abseil } // namespace tidy } // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index ecac660..1f6889a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -67,6 +67,12 @@ The improvements are... Improvements to clang-tidy -------------------------- +- New :doc:`abseil-duration-addition + ` check. + + Checks for cases where addition should be performed in the ``absl::Time`` + domain. + - New :doc:`abseil-duration-conversion-cast ` check. @@ -80,7 +86,6 @@ Improvements to clang-tidy Checks whether there are underscores in googletest test and test case names in test macros, which is prohibited by the Googletest FAQ. - Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-addition.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-addition.rst new file mode 100644 index 0000000..2f3d805 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-addition.rst @@ -0,0 +1,21 @@ +.. title:: clang-tidy - abseil-duration-addition + +abseil-duration-addition +======================== + +Check for cases where addition should be performed in the ``absl::Time`` domain. +When adding two values, and one is known to be an ``absl::Time``, we can infer +that the other should be interpreted as an ``absl::Duration`` of a similar +scale, and make that inference explicit. + +Examples: + +.. code-block:: c++ + + // Original - Addition in the integer domain + int x; + absl::Time t; + int result = absl::ToUnixSeconds(t) + x; + + // Suggestion - Addition in the absl::Time domain + int result = absl::TounixSeconds(t + absl::Seconds(x)); diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 8397675..487f682 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -4,6 +4,7 @@ Clang-Tidy Checks ================= .. toctree:: + abseil-duration-addition abseil-duration-comparison abseil-duration-conversion-cast abseil-duration-division diff --git a/clang-tools-extra/test/clang-tidy/Inputs/absl/time/time.h b/clang-tools-extra/test/clang-tidy/Inputs/absl/time/time.h index 9d136a5d..385eed8 100644 --- a/clang-tools-extra/test/clang-tidy/Inputs/absl/time/time.h +++ b/clang-tools-extra/test/clang-tidy/Inputs/absl/time/time.h @@ -55,6 +55,19 @@ int64_t ToInt64Milliseconds(Duration d); int64_t ToInt64Microseconds(Duration d); int64_t ToInt64Nanoseconds(Duration d); +int64_t ToUnixHours(Time t); +int64_t ToUnixMinutes(Time t); +int64_t ToUnixSeconds(Time t); +int64_t ToUnixMillis(Time t); +int64_t ToUnixMicros(Time t); +int64_t ToUnixNanos(Time t); +Time FromUnixHours(int64_t); +Time FromUnixMinutes(int64_t); +Time FromUnixSeconds(int64_t); +Time FromUnixMillis(int64_t); +Time FromUnixMicros(int64_t); +Time FromUnixNanos(int64_t); + // Relational Operators constexpr bool operator<(Duration lhs, Duration rhs); constexpr bool operator>(Duration lhs, Duration rhs); diff --git a/clang-tools-extra/test/clang-tidy/abseil-duration-addition.cpp b/clang-tools-extra/test/clang-tidy/abseil-duration-addition.cpp new file mode 100644 index 0000000..33cfc58 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/abseil-duration-addition.cpp @@ -0,0 +1,98 @@ +// RUN: %check_clang_tidy %s abseil-duration-addition %t -- -- -I%S/Inputs + +#include "absl/time/time.h" + +void f() { + absl::Time t; + int i; + + i = absl::ToUnixHours(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixHours(t + absl::Hours(5)) + i = absl::ToUnixMinutes(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMinutes(t + absl::Minutes(5)) + i = absl::ToUnixSeconds(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixSeconds(t + absl::Seconds(5)) + i = absl::ToUnixMillis(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMillis(t + absl::Milliseconds(5)) + i = absl::ToUnixMicros(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMicros(t + absl::Microseconds(5)) + i = absl::ToUnixNanos(t) + 5; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixNanos(t + absl::Nanoseconds(5)) + + i = 3 + absl::ToUnixHours(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixHours(absl::Hours(3) + t) + i = 3 + absl::ToUnixMinutes(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMinutes(absl::Minutes(3) + t) + i = 3 + absl::ToUnixSeconds(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixSeconds(absl::Seconds(3) + t) + i = 3 + absl::ToUnixMillis(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMillis(absl::Milliseconds(3) + t) + i = 3 + absl::ToUnixMicros(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMicros(absl::Microseconds(3) + t) + i = 3 + absl::ToUnixNanos(t); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixNanos(absl::Nanoseconds(3) + t) + + // Undoing inverse conversions + i = absl::ToUnixMicros(t) + absl::ToInt64Microseconds(absl::Seconds(1)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixMicros(t + absl::Seconds(1)) + + // Parens + i = 3 + (absl::ToUnixHours(t)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixHours(absl::Hours(3) + t) + + // Float folding + i = absl::ToUnixSeconds(t) + 5.0; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixSeconds(t + absl::Seconds(5)) + + // We can rewrite the argument of the duration conversion +#define THIRTY absl::FromUnixSeconds(30) + i = absl::ToUnixSeconds(THIRTY) + 1; + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixSeconds(THIRTY + absl::Seconds(1)) +#undef THIRTY + + // Some other contexts + if (absl::ToUnixSeconds(t) + 1.0 > 10) {} + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixSeconds(t + absl::Seconds(1)) + + // These should not match + i = 5 + 6; + i = absl::ToUnixSeconds(t) - 1.0; + i = absl::ToUnixSeconds(t) * 1.0; + i = absl::ToUnixSeconds(t) / 1.0; + i += absl::ToInt64Microseconds(absl::Seconds(1)); + +#define PLUS_FIVE(z) absl::ToUnixSeconds(z) + 5 + i = PLUS_FIVE(t); +#undef PLUS_FIVE +} + +// Within a templated function +template +void foo(absl::Time t) { + int i = absl::ToUnixNanos(t) + T{}; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: perform addition in the duration domain [abseil-duration-addition] + // CHECK-FIXES: absl::ToUnixNanos(t + absl::Nanoseconds(T{})) +} + +void g() { + absl::Time t; + foo(t); + foo(t); +} -- 2.7.4