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,104 @@ +//===--- 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 { + +namespace { + +// Returns an integer if the fractional part of a `FloatingLiteral` is 0. +llvm::Optional +TruncateIfIntegral(const FloatingLiteral &FloatLiteral) { + double value = FloatLiteral.getValueAsApproximateDouble(); + if (std::fmod(value, 1) == 0) { + bool is_negative = false; + if (value < 0) { + value = -value; + is_negative = true; + } + if (value >= static_cast(1u << 31)) + return llvm::None; + llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false); + ApInt = static_cast(value); + if (is_negative) + ApInt = -ApInt; + return ApInt; + } + return llvm::None; +} + +} // namespace + +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 (MatchedCall->getBeginLoc().isMacroID() || + MatchedCall->getEndLoc().isMacroID()) + return; + + const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts(); + // We don't mess with macros. + 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 FloatingLiteral *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 @@ -67,6 +67,15 @@ Improvements to clang-tidy -------------------------- +- New :doc:`abseil-duration-factory-float + ` check. + + FIXME: add release notes. + + 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-duration-division ` 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,12 +5,13 @@ .. toctree:: abseil-duration-division + abseil-duration-factory-float abseil-faster-strsplit-delimiter abseil-no-internal-dependencies abseil-no-namespace abseil-redundant-strcat-calls - abseil-string-find-startswith abseil-str-cat-append + abseil-string-find-startswith android-cloexec-accept android-cloexec-accept4 android-cloexec-creat Index: test/clang-tidy/abseil-duration-factory-float.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-duration-factory-float.cpp @@ -0,0 +1,104 @@ +// 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); + + // Ignored expressions + d = absl::Seconds(.001); + d = absl::Seconds(.100); + d = ::absl::Seconds(1); + d = ::absl::Minutes(1); + d = ::absl::Hours(1); + + // 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)); +}