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 funciton 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,293 @@ +//===--- 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 { + +// Potential scales of our inputs. +enum class DurationScale { + Hours, + Minutes, + Seconds, + Milliseconds, + Microseconds, + Nanoseconds, +}; + +// 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 auto *ScaleMap = + new std::unordered_map( + {{"Nanoseconds", DurationScale::Nanoseconds}, + {"Microseconds", DurationScale::Microseconds}, + {"Milliseconds", DurationScale::Milliseconds}, + {"Seconds", DurationScale::Seconds}, + {"Minutes", DurationScale::Minutes}, + {"Hours", DurationScale::Hours}}); + + const auto ScaleIter = ScaleMap->find(FactoryName); + if (ScaleIter == ScaleMap->end()) { + return llvm::None; + } + + return ScaleIter->second; +} + +// Given either an integer or float literal, return it's 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(); + } else { + assert(FloatLit != nullptr); + return FloatLit->getValueAsApproximateDouble(); + } +} + +// 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 GetNewMultScale(DurationScale OldScale, + double Multiplier) { + switch (OldScale) { + case DurationScale::Hours: + // We can't scale Hours. + break; + + case DurationScale::Minutes: + if (Multiplier == 60.0) + return DurationScale::Hours; + break; + + case DurationScale::Seconds: + if (Multiplier == 60.0) + return DurationScale::Minutes; + else if (Multiplier == 1e-3) + return DurationScale::Milliseconds; + break; + + case DurationScale::Milliseconds: + if (Multiplier == 1e3) + return DurationScale::Seconds; + else if (Multiplier == 1e-3) + return DurationScale::Microseconds; + break; + + case DurationScale::Microseconds: + if (Multiplier == 1e3) + return DurationScale::Milliseconds; + else if (Multiplier == 1e-6) + return DurationScale::Nanoseconds; + break; + + case DurationScale::Nanoseconds: + if (Multiplier == 1e3) + return DurationScale::Microseconds; + break; + } + + return llvm::None; +} + +// Given the scale of a duration and `Divisor`, determine if division by +// `Divisor` would produce a new scale. If so, return it, otherwise `None`. +static llvm::Optional GetNewDivScale(DurationScale OldScale, + double Divisor) { + switch (OldScale) { + case DurationScale::Hours: + if (Divisor == 60.0) + return DurationScale::Minutes; + break; + + case DurationScale::Minutes: + if (Divisor == 60.0) + return DurationScale::Seconds; + break; + + case DurationScale::Seconds: + if (Divisor == 1e3) + return DurationScale::Milliseconds; + break; + + case DurationScale::Milliseconds: + if (Divisor == 1e3) + return DurationScale::Microseconds; + break; + + case DurationScale::Microseconds: + if (Divisor == 1e3) + return DurationScale::Nanoseconds; + break; + + case DurationScale::Nanoseconds: + // We can't scale Nanoseconds down. + break; + } + + return llvm::None; +} + +// Given a `Scale`, return the appropriate factory function call for +// constructing a `Duration` for that scale. +static std::string 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"; + } + return "unreachable"; +} + +// Returns `true` if `Range` is inside a macro definition. +static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result, + SourceRange Range) { + return !clang::Lexer::makeFileCharRange( + clang::CharSourceRange::getCharRange(Range), + *Result.SourceManager, Result.Context->getLangOpts()) + .isValid(); +} + +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 and replace things inside of macro definitions. + if (InsideMacroDefinition(Result, Call->getSourceRange())) + 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"); + auto MaybeScale = GetScaleForFactory(CallDecl->getName()); + if (!MaybeScale) + return; + + DurationScale Scale = *MaybeScale; + + // We next handle the cases of multication. + // We need to look at both operands, and consider the cases where a user + // is multiplying by something such as 1e-3. + const auto *MultBinOp = Result.Nodes.getNodeAs("mult_binop"); + if (MultBinOp) { + const Expr *Remainder; + llvm::Optional NewScale; + + // First check the LHS + const auto *IntLit = llvm::dyn_cast(MultBinOp->getLHS()); + const auto *FloatLit = llvm::dyn_cast(MultBinOp->getLHS()); + if (IntLit || FloatLit) { + NewScale = GetNewMultScale(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 = GetNewMultScale(Scale, GetValue(IntLit, FloatLit)); + if (NewScale) + Remainder = MultBinOp->getLHS(); + } + } + + if (NewScale) { + assert(Remainder); + // 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; + } + + // We next handle division. + const auto *DivBinOp = Result.Nodes.getNodeAs("div_binop"); + if (DivBinOp) { + DivBinOp->dumpColor(); + // For division, we only check the RHS. + const auto *FloatLit = llvm::dyn_cast(DivBinOp->getRHS()); + + llvm::Optional NewScale = + GetNewDivScale(Scale, 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()); + } + } + + // We can't do anything with this expression, so return. + return; +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -81,6 +81,14 @@ ``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. This + check also looks for arguements with a zero value and suggests using + ``absl::ZeroDuration()`` instead. + - 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,101 @@ +// 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(); + + // 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); + + // 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); + + // 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); +}