Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "DurationDivisionCheck.h" +#include "DurationFactoryFloatCheck.h" #include "FasterStrsplitDelimiterCheck.h" #include "NoInternalDependenciesCheck.h" #include "NoNamespaceCheck.h" @@ -27,6 +28,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "abseil-duration-division"); + CheckFactories.registerCheck( + "abseil-duration-factory-float"); CheckFactories.registerCheck( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck( Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -3,6 +3,7 @@ add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp DurationDivisionCheck.cpp + DurationFactoryFloatCheck.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp Index: clang-tidy/abseil/DurationFactoryFloatCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationFactoryFloatCheck.h @@ -0,0 +1,35 @@ +//===--- DurationFactoryFloatCheck.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_DURATIONFACTORYFLOATCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONFACTORYFLOATCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Prefer integer Duration factories when possible. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-factory-float.html +class DurationFactoryFloatCheck : public ClangTidyCheck { +public: + DurationFactoryFloatCheck(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_DURATIONFACTORYFLOATCHECK_H Index: clang-tidy/abseil/DurationFactoryFloatCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/DurationFactoryFloatCheck.cpp @@ -0,0 +1,101 @@ +//===--- DurationFactoryFloatCheck.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 "DurationFactoryFloatCheck.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 { + +// 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; + + llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false); + ApInt = static_cast(Value); + return ApInt; + } + return llvm::None; +} + +static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result, + SourceRange Range) { + return !clang::Lexer::makeFileCharRange( + clang::CharSourceRange::getCharRange(Range), + *Result.SourceManager, Result.Context->getLangOpts()) + .isValid(); +} + +void DurationFactoryFloatCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + 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"))), + floatLiteral().bind("float_literal")))) + .bind("call"), + this); +} + +void DurationFactoryFloatCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedCall = Result.Nodes.getNodeAs("call"); + + // Don't try and replace things inside of macro definitions. + if (InsideMacroDefinition(Result, MatchedCall->getSourceRange())) + return; + + const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts(); + // Macros are ignored. + if (Arg->getBeginLoc().isMacroID()) + return; + + // Check for static casts to float. + if (const auto *MaybeCastArg = Result.Nodes.getNodeAs("cast_arg")) { + diag(MatchedCall->getBeginLoc(), + (llvm::Twine("Use 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 integer version of absl::") + + MatchedCall->getDirectCallee()->getName()) + .str()) + << FixItHint::CreateReplacement(LitFloat->getSourceRange(), + IntValue->toString(/*radix=*/10)); + return; + } + } +} + +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -74,6 +74,13 @@ floating-point context, and recommends the use of a function that returns a floating-point value. +- New :doc:`abseil-duration-factory-float + ` check. + + Checks for cases where the floating-point overloads of various + ``absl::Duration`` factory functions is called when the more-efficient + integer version could be used instead. + - New :doc:`abseil-faster-strsplit-delimiter ` check. Index: docs/clang-tidy/checks/abseil-duration-factory-float.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-duration-factory-float.rst @@ -0,0 +1,29 @@ +.. title:: clang-tidy - abseil-duration-factory-float + +abseil-duration-factory-float +============================= + +Finds cases where callers of ``absl::Duration`` factory functions (such as +``absl::Seconds`` or ``absl::Hours``) are providing a floating point value +when an integer could be used instead. The integer factory function overload +is more efficient and readable. + +This check will not suggest fixes for fractional floating point values or +non-literals. + +Examples: + +.. code-block:: c++ + + // Original - Providing a floating-point literal. + absl::Duration d = absl::Seconds(10.0); + + // Suggested - Use an integer instead. + absl::Duration d = absl::Seconds(10); + + + // Original - Explicitly casting to a floating-point type. + absl::Duration d = absl::Seconds(static_cast(10)); + + // Suggested - Remove the explicit cast + absl::Duration d = absl::Seconds(10); Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -5,6 +5,7 @@ .. toctree:: abseil-duration-division + abseil-duration-factory-float abseil-faster-strsplit-delimiter abseil-no-internal-dependencies abseil-no-namespace Index: test/clang-tidy/abseil-duration-factory-float.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-duration-factory-float.cpp @@ -0,0 +1,115 @@ +// RUN: %check_clang_tidy %s abseil-duration-factory-float %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 ConvertFloatTest() { + absl::Duration d; + + d = absl::Seconds(60.0); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(60); + d = absl::Minutes(300.0); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(300); + + d = absl::Milliseconds(1e2); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Milliseconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Milliseconds(100); + d = absl::Seconds(3.0f); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(3); + d = absl::Seconds(3.); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(3); + d = absl::Seconds(0x3.p0); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(3); + d = absl::Seconds(0x3.p1); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(6); + + + // Ignored expressions + d = absl::Seconds(.001); + d = absl::Seconds(.100); + d = ::absl::Seconds(1); + d = ::absl::Minutes(1); + d = ::absl::Hours(1); + d = absl::Seconds(0x3.4p1); + + // Negative literals (we don't yet handle this case) + d = absl::Seconds(-3.0); + + // This is bigger than we can safely fit in a positive int32, so we ignore it. + d = absl::Seconds(1e12); + + int x; + d = absl::Seconds(x); + float y; + d = absl::Minutes(y); + +#define SECONDS(x) absl::Seconds(x) + SECONDS(60); +#undef SECONDS +#define THIRTY 30.0 + d = absl::Seconds(THIRTY); +#undef THIRTY +} + +template +void InTemplate() { + absl::Duration d; + + d = absl::Seconds(N); // 1 + // ^ No replacement here. + + d = absl::Minutes(1.0); // 2 + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(1); // 2 +} + +void Instantiate() { + InTemplate<60>(); + InTemplate<1>(); +} + +void ConvertStaticCastTest() { + absl::Duration d; + + d = absl::Seconds(static_cast(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(5); + + d = absl::Minutes(static_cast(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: Use integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(5); + + // This should not be flagged + d = absl::Seconds(static_cast(5.0)); +}