diff options
8 files changed, 318 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index f9ed4e1bd9e..e02b1e0e3c5 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/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 @@ public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck<DurationDivisionCheck>( "abseil-duration-division"); + CheckFactories.registerCheck<DurationFactoryFloatCheck>( + "abseil-duration-factory-float"); CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>( "abseil-faster-strsplit-delimiter"); CheckFactories.registerCheck<NoInternalDependenciesCheck>( diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index e3191b74f01..4987bfa0a42 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -3,6 +3,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp DurationDivisionCheck.cpp + DurationFactoryFloatCheck.cpp FasterStrsplitDelimiterCheck.cpp NoInternalDependenciesCheck.cpp NoNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.cpp new file mode 100644 index 00000000000..80ecf8a812e --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.cpp @@ -0,0 +1,106 @@ +//===--- 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<llvm::APSInt> +truncateIfIntegral(const FloatingLiteral &FloatLiteral) { + double Value = FloatLiteral.getValueAsApproximateDouble(); + if (std::fmod(Value, 1) == 0) { + if (Value >= static_cast<double>(1u << 31)) + return llvm::None; + + return llvm::APSInt::get(static_cast<int64_t>(Value)); + } + return llvm::None; +} + +// 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 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"))), + cStyleCastExpr( + hasDestinationType(realFloatingPointType()), + hasSourceExpression(expr().bind("cast_arg"))), + cxxFunctionalCastExpr( + 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<CallExpr>("call"); + + // Don't try and replace things inside of macro definitions. + if (InsideMacroDefinition(Result, MatchedCall->getSourceRange())) + return; + + const Expr *Arg = MatchedCall->getArg(0)->IgnoreImpCasts(); + // Arguments which are macros are ignored. + if (Arg->getBeginLoc().isMacroID()) + return; + + // Check for casts to `float` or `double`. + if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) { + diag(MatchedCall->getBeginLoc(), + (llvm::Twine("use the 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<FloatingLiteral>("float_literal")) { + // Attempt to simplify a `Duration` factory call with a literal argument. + if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat)) { + diag(MatchedCall->getBeginLoc(), + (llvm::Twine("use the integer version of absl::") + + MatchedCall->getDirectCallee()->getName()) + .str()) + << FixItHint::CreateReplacement(LitFloat->getSourceRange(), + IntValue->toString(/*radix=*/10)); + return; + } + } +} + +} // namespace abseil +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.h b/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.h new file mode 100644 index 00000000000..8d215e4456d --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.h @@ -0,0 +1,38 @@ +//===--- 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 { + +/// This check finds cases where `Duration` factories are being called with +/// floating point arguments, but could be called using integer arguments. +/// It handles explicit casts and floating point literals with no fractional +/// component. +/// +/// 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7b70216b81d..4b1a6f2b7c8 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -74,6 +74,13 @@ Improvements to clang-tidy floating-point context, and recommends the use of a function that returns a floating-point value. +- New :doc:`abseil-duration-factory-float + <clang-tidy/checks/abseil-duration-factory-float>` check. + + Checks for cases where the floating-point overloads of various + ``absl::Duration`` factory functions are called when the more-efficient + integer versions could be used instead. + - New :doc:`abseil-faster-strsplit-delimiter <clang-tidy/checks/abseil-faster-strsplit-delimiter>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-factory-float.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-factory-float.rst new file mode 100644 index 00000000000..a4fc6d4291d --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-duration-factory-float.rst @@ -0,0 +1,29 @@ +.. title:: clang-tidy - abseil-duration-factory-float + +abseil-duration-factory-float +============================= + +Checks for cases where the floating-point overloads of various +``absl::Duration`` factory functions are called when the more-efficient +integer versions could be used instead. + +This check will not suggest fixes for literals which contain fractional +floating point values or non-literals. It will suggest removing +superfluous casts. + +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<double>(10)); + + // Suggested - Remove the explicit cast + absl::Duration d = absl::Seconds(10); diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index f7fa7c5a33f..a5b5a4f7780 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -5,6 +5,7 @@ Clang-Tidy Checks .. toctree:: abseil-duration-division + abseil-duration-factory-float abseil-faster-strsplit-delimiter abseil-no-internal-dependencies abseil-no-namespace diff --git a/clang-tools-extra/test/clang-tidy/abseil-duration-factory-float.cpp b/clang-tools-extra/test/clang-tidy/abseil-duration-factory-float.cpp new file mode 100644 index 00000000000..443eed32abf --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/abseil-duration-factory-float.cpp @@ -0,0 +1,133 @@ +// 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 <typename T> \ + 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 the 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 the 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 the 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 the 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 the 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 the 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 the 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 <int N> +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 the integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(1); // 2 +} + +void Instantiate() { + InTemplate<60>(); + InTemplate<1>(); +} + +void ConvertCastTest() { + absl::Duration d; + + d = absl::Seconds(static_cast<double>(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(5); + + d = absl::Minutes(static_cast<float>(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(5); + + d = absl::Seconds((double) 5); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(5); + + d = absl::Minutes((float) 5); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(5); + + d = absl::Seconds(double(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Seconds [abseil-duration-factory-float] + // CHECK-FIXES: absl::Seconds(5); + + d = absl::Minutes(float(5)); + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: use the integer version of absl::Minutes [abseil-duration-factory-float] + // CHECK-FIXES: absl::Minutes(5); + + // This should not be flagged + d = absl::Seconds(static_cast<int>(5.0)); + d = absl::Seconds((int) 5.0); + d = absl::Seconds(int(5.0)); +} |
