[clang-tidy] Add the abseil-duration-factory-float check

Summary:
This check finds cases where calls to an absl::Duration factory could use the more efficient integer overload.

For example:
// Original - Providing a floating-point literal.
absl::Duration d = absl::Seconds(10.0);

// Suggested - Use an integer instead.
absl::Duration d = absl::Seconds(10);

Patch by hwright.

Reviewers: alexfh, hokein, aaron.ballman, JonasToth

Reviewed By: hokein, JonasToth

Subscribers: zturner, xazax.hun, Eugene.Zelenko, mgorny, cfe-commits

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D53339

llvm-svn: 345167

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>(

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

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

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

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.
 

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);

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

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));
+}