Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -12,6 +12,7 @@ #include "../ClangTidyModuleRegistry.h" #include "DurationDivisionCheck.h" #include "DurationFactoryFloatCheck.h" +#include "DurationFactoryScaleCheck.h" #include "FasterStrsplitDelimiterCheck.h" #include "NoInternalDependenciesCheck.h" #include "NoNamespaceCheck.h" @@ -30,6 +31,8 @@ "abseil-duration-division"); CheckFactories.registerCheck( "abseil-duration-factory-float"); + CheckFactories.registerCheck( + "abseil-duration-factory-scale"); CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck( Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -4,6 +4,7 @@ AbseilTidyModule.cpp DurationDivisionCheck.cpp DurationFactoryFloatCheck.cpp + DurationFactoryScaleCheck.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp Index: clang-tidy/abseil/DurationFactoryScaleCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationFactoryScaleCheck.h @@ -0,0 +1,38 @@ +//===--- DurationFactoryScaleCheck.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_DURATIONFACTORYSCALECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYSCALECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// This check finds cases where the incorrect `Duration` factory function is +/// being used by looking for scaling constants inside the factory argument +/// and suggesting a more appropriate factory. It also looks for the special +/// case of zero and suggests `ZeroDuration()`. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-factory-scale.html +class DurationFactoryScaleCheck : public ClangTidyCheck { +public: + DurationFactoryScaleCheck(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_DURATIONFACTORYSCALECHECK_H Index: clang-tidy/abseil/DurationFactoryScaleCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationFactoryScaleCheck.cpp @@ -0,0 +1,269 @@ +//===--- DurationFactoryScaleCheck.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 "DurationFactoryScaleCheck.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 { + +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`. +static llvm::Optional +getScaleForFactory(llvm::StringRef FactoryName) { + static const std::unordered_map ScaleMap( + {{"Nanoseconds", DurationScale::Nanoseconds}, + {"Microseconds", DurationScale::Microseconds}, + {"Milliseconds", DurationScale::Milliseconds}, + {"Seconds", DurationScale::Seconds}, + {"Minutes", DurationScale::Minutes}, + {"Hours", DurationScale::Hours}}); + + auto ScaleIter = ScaleMap.find(FactoryName); + if (ScaleIter == ScaleMap.end()) + return llvm::None; + + return ScaleIter->second; +} + +// Given either an integer or float literal, return its value. +// One and only one of `IntLit` and `FloatLit` should be provided. +static double GetValue(const IntegerLiteral *IntLit, + const FloatingLiteral *FloatLit) { + if (IntLit) + return IntLit->getValue().getLimitedValue(); + + assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set"); + return FloatLit->getValueAsApproximateDouble(); +} + +// Given the scale of a duration and a `Multiplier`, determine if `Multiplier` +// would produce a new scale. If so, return a tuple containing the new scale +// and a suitable Multipler for that scale, otherwise `None`. +static llvm::Optional> +GetNewScaleSingleStep(DurationScale OldScale, double Multiplier) { + switch (OldScale) { + case DurationScale::Hours: + if (Multiplier <= 1.0 / 60.0) + return std::make_tuple(DurationScale::Minutes, Multiplier * 60.0); + break; + + case DurationScale::Minutes: + if (Multiplier >= 60.0) + return std::make_tuple(DurationScale::Hours, Multiplier / 60.0); + if (Multiplier <= 1.0 / 60.0) + return std::make_tuple(DurationScale::Seconds, Multiplier * 60.0); + break; + + case DurationScale::Seconds: + if (Multiplier >= 60.0) + return std::make_tuple(DurationScale::Minutes, Multiplier / 60.0); + if (Multiplier <= 1e-3) + return std::make_tuple(DurationScale::Milliseconds, Multiplier * 1e3); + break; + + case DurationScale::Milliseconds: + if (Multiplier >= 1e3) + return std::make_tuple(DurationScale::Seconds, Multiplier / 1e3); + if (Multiplier <= 1e-3) + return std::make_tuple(DurationScale::Microseconds, Multiplier * 1e3); + break; + + case DurationScale::Microseconds: + if (Multiplier >= 1e3) + return std::make_tuple(DurationScale::Milliseconds, Multiplier / 1e3); + if (Multiplier <= 1e-3) + return std::make_tuple(DurationScale::Nanoseconds, Multiplier * 1e-3); + break; + + case DurationScale::Nanoseconds: + if (Multiplier >= 1e3) + return std::make_tuple(DurationScale::Microseconds, Multiplier / 1e3); + break; + } + + return llvm::None; +} + +// Given the scale of a duration and a `Multiplier`, determine if `Multiplier` +// would produce a new scale. If so, return it, otherwise `None`. +static llvm::Optional GetNewScale(DurationScale OldScale, + double Multiplier) { + while (Multiplier != 1.0) { + llvm::Optional> result = + GetNewScaleSingleStep(OldScale, Multiplier); + if (!result) + break; + if (std::get<1>(*result) == 1.0) + return std::get<0>(*result); + Multiplier = std::get<1>(*result); + OldScale = std::get<0>(*result); + } + + 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(); +} + +void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + callee(functionDecl( + hasAnyName("::absl::Nanoseconds", "::absl::Microseconds", + "::absl::Milliseconds", "::absl::Seconds", + "::absl::Minutes", "::absl::Hours")) + .bind("call_decl")), + hasArgument( + 0, + ignoringImpCasts(anyOf( + integerLiteral(equals(0)).bind("zero"), + floatLiteral(equals(0.0)).bind("zero"), + binaryOperator(hasOperatorName("*"), + hasEitherOperand(ignoringImpCasts( + anyOf(integerLiteral(), floatLiteral())))) + .bind("mult_binop"), + binaryOperator(hasOperatorName("/"), hasRHS(floatLiteral())) + .bind("div_binop"))))) + .bind("call"), + this); +} + +void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs("call"); + + // Don't try to replace things inside of macro definitions. + if (Call->getExprLoc().isMacroID()) + return; + + const Expr *Arg = Call->getArg(0)->IgnoreParenImpCasts(); + // Arguments which are macros are ignored. + if (Arg->getBeginLoc().isMacroID()) + return; + + // We first handle the cases of literal zero (both float and integer). + if (Result.Nodes.getNodeAs("zero")) { + diag(Call->getBeginLoc(), + "use ZeroDuration() for zero-length time intervals") + << FixItHint::CreateReplacement(Call->getSourceRange(), + "absl::ZeroDuration()"); + return; + } + + const auto *CallDecl = Result.Nodes.getNodeAs("call_decl"); + llvm::Optional MaybeScale = + getScaleForFactory(CallDecl->getName()); + if (!MaybeScale) + return; + + DurationScale Scale = *MaybeScale; + const Expr *Remainder; + llvm::Optional NewScale; + + // We next handle the cases of multiplication and division. + if (const auto *MultBinOp = + Result.Nodes.getNodeAs("mult_binop")) { + // For multiplication, we need to look at both operands, and consider the + // cases where a user is multiplying by something such as 1e-3. + + // First check the LHS + const auto *IntLit = llvm::dyn_cast(MultBinOp->getLHS()); + const auto *FloatLit = llvm::dyn_cast(MultBinOp->getLHS()); + if (IntLit || FloatLit) { + NewScale = GetNewScale(Scale, GetValue(IntLit, FloatLit)); + if (NewScale) + Remainder = MultBinOp->getRHS(); + } + + // If we weren't able to scale based on the LHS, check the RHS + if (!NewScale) { + IntLit = llvm::dyn_cast(MultBinOp->getRHS()); + FloatLit = llvm::dyn_cast(MultBinOp->getRHS()); + if (IntLit || FloatLit) { + NewScale = GetNewScale(Scale, GetValue(IntLit, FloatLit)); + if (NewScale) + Remainder = MultBinOp->getLHS(); + } + } + } else if (const auto *DivBinOp = + Result.Nodes.getNodeAs("div_binop")) { + // We next handle division. + // For division, we only check the RHS. + const auto *FloatLit = llvm::dyn_cast(DivBinOp->getRHS()); + + llvm::Optional NewScale = + GetNewScale(Scale, 1.0 / FloatLit->getValueAsApproximateDouble()); + if (NewScale) { + const Expr *Remainder = DivBinOp->getLHS(); + + // We've found an appropriate scaling factor and the new scale, so output + // the relevant fix. + diag(Call->getBeginLoc(), "internal duration scaling can be removed") + << FixItHint::CreateReplacement( + Call->getSourceRange(), + (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" + + tooling::fixit::getText(*Remainder, *Result.Context) + ")") + .str()); + } + } + + if (NewScale) { + assert(Remainder && "No remainder found"); + // We've found an appropriate scaling factor and the new scale, so output + // the relevant fix. + diag(Call->getBeginLoc(), "internal duration scaling can be removed") + << FixItHint::CreateReplacement( + Call->getSourceRange(), + (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" + + tooling::fixit::getText(*Remainder, *Result.Context) + ")") + .str()); + } + return; +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -81,6 +81,12 @@ ``absl::Duration`` factory functions are called when the more-efficient integer versions could be used instead. +- New :doc:`abseil-duration-factory-scale + ` check. + + 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-faster-strsplit-delimiter ` check. Index: docs/clang-tidy/checks/abseil-duration-factory-scale.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-duration-factory-scale.rst @@ -0,0 +1,35 @@ +.. title:: clang-tidy - abseil-duration-factory-scale + +abseil-duration-factory-scale +============================= + +Checks for cases where arguments to ``absl::Duration`` factory functions are +scaled internally and could be changed to a different factory function. This +check also looks for arguements with a zero value and suggests using +``absl::ZeroDuration()`` instead. + +Examples: + +.. code-block:: c++ + + // Original - Internal multiplication. + int x; + absl::Duration d = absl::Seconds(60 * x); + + // Suggested - Use absl::Minutes instead. + absl::Duration d = absl::Minutes(x); + + + // Original - Internal division. + int y; + absl::Duration d = absl::Milliseconds(y / 1000.); + + // Suggested - Use absl:::Seconds instead. + absl::Duration d = absl::Seconds(y); + + + // Original - Zero-value argument. + absl::Duration d = absl::Hours(0); + + // Suggested = Use absl::ZeroDuration instead + absl::Duration d = absl::ZeroDuration(); Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -6,6 +6,7 @@ .. toctree:: abseil-duration-division abseil-duration-factory-float + abseil-duration-factory-scale abseil-faster-strsplit-delimiter abseil-no-internal-dependencies abseil-no-namespace Index: test/clang-tidy/abseil-duration-factory-scale.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-duration-factory-scale.cpp @@ -0,0 +1,130 @@ +// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t + +// Mimic the implementation of absl::Duration +namespace absl { + +class Duration {}; + +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 + +} // namespace absl + +void ScaleTest() { + absl::Duration d; + + // Zeroes + d = absl::Hours(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Minutes(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Seconds(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Milliseconds(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Microseconds(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Nanoseconds(0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Seconds(0.0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + d = absl::Seconds(0x0.000001p-126f); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale] + // CHECK-FIXES: absl::ZeroDuration(); + + // Fold seconds into minutes + d = absl::Seconds(30 * 60); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Minutes(30); + d = absl::Seconds(60 * 30); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Minutes(30); + + // Try a few more exotic multiplications + d = absl::Seconds(60 * 30 * 60); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Minutes(60 * 30); + d = absl::Seconds(1e-3 * 30); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Milliseconds(30); + d = absl::Milliseconds(30 * 1000); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Seconds(30); + d = absl::Milliseconds(30 * 0.001); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Microseconds(30); + + // Multiple steps + d = absl::Seconds(5 * 3600); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Hours(5); + d = absl::Microseconds(5 * 1e6); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Seconds(5); + d = absl::Seconds(5 * 1e-6); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Microseconds(5); + d = absl::Microseconds(5 * 1000000); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Seconds(5); + + // Division + d = absl::Hours(30 / 60.); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Minutes(30); + d = absl::Seconds(30 / 1000.); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Milliseconds(30); + d = absl::Milliseconds(30 / 1e3); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Microseconds(30); + d = absl::Seconds(30 / 1e6); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale] + // CHECK-FIXES: absl::Microseconds(30); + + // None of these should trigger the check + d = absl::Seconds(60); + d = absl::Seconds(60 + 30); + d = absl::Seconds(60 - 30); + d = absl::Seconds(50 * 30); + d = absl::Hours(60 * 60); + d = absl::Nanoseconds(1e-3 * 30); + d = absl::Seconds(1000 / 30); + // We don't support division by integers, which could cause rounding + d = absl::Seconds(10 / 1000); + d = absl::Seconds(30 / 50); + +#define EXPRESSION 30 * 60 + d = absl::Seconds(EXPRESSION); +#undef EXPRESSION + +// This should not trigger +#define HOURS(x) absl::Minutes(60 * x) + d = HOURS(40); +#undef HOURS +}