[clang-tidy] Add the abseil-duration-comparison check

Summary:
This check finds instances where Duration values are being converted to a numeric value in a comparison expression, and suggests that the conversion happen on the other side of the expression to a Duration.  See documentation for examples.

This also shuffles some code around so that the new check may perform in sone step simplifications also caught by other checks.

Patch by hwright.

Reviewers: aaron.ballman, JonasToth, alexfh, hokein

Reviewed By: JonasToth

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

Tags: #clang-tools-extra

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

llvm-svn: 348161

clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp

@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "DurationComparisonCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
@@ -27,6 +28,8 @@ namespace abseil {
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<DurationComparisonCheck>(
+        "abseil-duration-comparison");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(

clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

@@ -2,9 +2,11 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  DurationComparisonCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
+  DurationRewriter.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp

@@ -0,0 +1,164 @@
+//===--- DurationComparisonCheck.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 "DurationComparisonCheck.h"
+#include "DurationRewriter.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 {
+
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
+      {{"ToDoubleHours", DurationScale::Hours},
+       {"ToInt64Hours", DurationScale::Hours},
+       {"ToDoubleMinutes", DurationScale::Minutes},
+       {"ToInt64Minutes", DurationScale::Minutes},
+       {"ToDoubleSeconds", DurationScale::Seconds},
+       {"ToInt64Seconds", DurationScale::Seconds},
+       {"ToDoubleMilliseconds", DurationScale::Milliseconds},
+       {"ToInt64Milliseconds", DurationScale::Milliseconds},
+       {"ToDoubleMicroseconds", DurationScale::Microseconds},
+       {"ToInt64Microseconds", DurationScale::Microseconds},
+       {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
+       {"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
+
+  auto ScaleIter = ScaleMap.find(std::string(Name));
+  if (ScaleIter == ScaleMap.end())
+    return llvm::None;
+
+  return ScaleIter->second;
+}
+
+/// Given a `Scale` return the inverse functions for it.
+static const std::pair<llvm::StringRef, llvm::StringRef> &
+getInverseForScale(DurationScale Scale) {
+  static const std::unordered_map<DurationScale,
+                                  std::pair<llvm::StringRef, llvm::StringRef>>
+      InverseMap(
+          {{DurationScale::Hours,
+            std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},
+           {DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes",
+                                                   "::absl::ToInt64Minutes")},
+           {DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds",
+                                                   "::absl::ToInt64Seconds")},
+           {DurationScale::Milliseconds,
+            std::make_pair("::absl::ToDoubleMilliseconds",
+                           "::absl::ToInt64Milliseconds")},
+           {DurationScale::Microseconds,
+            std::make_pair("::absl::ToDoubleMicroseconds",
+                           "::absl::ToInt64Microseconds")},
+           {DurationScale::Nanoseconds,
+            std::make_pair("::absl::ToDoubleNanoseconds",
+                           "::absl::ToInt64Nanoseconds")}});
+
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.
+  auto InverseIter = InverseMap.find(Scale);
+  assert(InverseIter != InverseMap.end() && "Unexpected scale found");
+  return InverseIter->second;
+}
+
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+                                DurationScale Scale, const Expr &Node) {
+  const std::pair<std::string, std::string> &InverseFunctions =
+      getInverseForScale(Scale);
+  if (const Expr *MaybeCallArg = selectFirst<const Expr>(
+          "e", match(callExpr(callee(functionDecl(
+                                  hasAnyName(InverseFunctions.first.c_str(),
+                                             InverseFunctions.second.c_str()))),
+                              hasArgument(0, expr().bind("e"))),
+                     Node, *Result.Context))) {
+    return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }
+
+  return llvm::None;
+}
+
+/// Assuming `Node` has type `double` or `int` representing a time interval of
+/// `Scale`, return the expression to make it a suitable `Duration`.
+static llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    const Expr *Node) {
+  const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+  if (RootNode.getBeginLoc().isMacroID())
+    return llvm::None;
+
+  // First check to see if we can undo a complimentary function call.
+  if (llvm::Optional<std::string> MaybeRewrite =
+          maybeRewriteInverseDurationCall(Result, Scale, RootNode))
+    return *MaybeRewrite;
+
+  if (IsLiteralZero(Result, RootNode))
+    return std::string("absl::ZeroDuration()");
+
+  return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
+          simplifyDurationFactoryArg(Result, RootNode) + ")")
+      .str();
+}
+
+void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher =
+      binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+                           hasOperatorName("=="), hasOperatorName("<="),
+                           hasOperatorName("<")),
+                     hasEitherOperand(ignoringImpCasts(callExpr(
+                         callee(functionDecl(DurationConversionFunction())
+                                    .bind("function_decl"))))))
+          .bind("binop");
+
+  Finder->addMatcher(Matcher, this);
+}
+
+void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID())
+    return;
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(
+      Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+  if (!Scale)
+    return;
+
+  // In most cases, we'll only need to rewrite one of the sides, but we also
+  // want to handle the case of rewriting both sides. This is much simpler if
+  // we unconditionally try and rewrite both, and let the rewriter determine
+  // if nothing needs to be done.
+  llvm::Optional<std::string> LhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
+  llvm::Optional<std::string> RhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+
+  if (!(LhsReplacement && RhsReplacement))
+    return;
+
+  diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
+      << FixItHint::CreateReplacement(Binop->getSourceRange(),
+                                      (llvm::Twine(*LhsReplacement) + " " +
+                                       Binop->getOpcodeStr() + " " +
+                                       *RhsReplacement)
+                                          .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.h

@@ -0,0 +1,36 @@
+//===--- DurationComparisonCheck.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_DURATIONCOMPARISONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the absl::Duration domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-comparison.html
+class DurationComparisonCheck : public ClangTidyCheck {
+public:
+  DurationComparisonCheck(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_DURATIONCOMPARISONCHECK_H

clang-tools-extra/clang-tidy/abseil/DurationFactoryFloatCheck.cpp

@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "DurationFactoryFloatCheck.h"
+#include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/FixIt.h"
@@ -18,19 +19,6 @@ 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) {
@@ -42,21 +30,14 @@ static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
 
 void DurationFactoryFloatCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      callExpr(
+      callExpr(callee(functionDecl(DurationFactoryFunction())),
-          callee(functionDecl(hasAnyName(
+               hasArgument(0, anyOf(cxxStaticCastExpr(hasDestinationType(
-              "absl::Nanoseconds", "absl::Microseconds", "absl::Milliseconds",
+                                        realFloatingPointType())),
-              "absl::Seconds", "absl::Minutes", "absl::Hours"))),
+                                    cStyleCastExpr(hasDestinationType(
-          hasArgument(0,
+                                        realFloatingPointType())),
-                      anyOf(cxxStaticCastExpr(
+                                    cxxFunctionalCastExpr(hasDestinationType(
-                                hasDestinationType(realFloatingPointType()),
+                                        realFloatingPointType())),
-                                hasSourceExpression(expr().bind("cast_arg"))),
+                                    floatLiteral())))
-                            cStyleCastExpr(
-                                hasDestinationType(realFloatingPointType()),
-                                hasSourceExpression(expr().bind("cast_arg"))),
-                            cxxFunctionalCastExpr(
-                                hasDestinationType(realFloatingPointType()),
-                                hasSourceExpression(expr().bind("cast_arg"))),
-                            floatLiteral().bind("float_literal"))))
           .bind("call"),
       this);
 }
@@ -73,31 +54,16 @@ void DurationFactoryFloatCheck::check(const MatchFinder::MatchResult &Result) {
   if (Arg->getBeginLoc().isMacroID())
     return;
 
-  // Check for casts to `float` or `double`.
+  llvm::Optional<std::string> SimpleArg = stripFloatCast(Result, *Arg);
-  if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
+  if (!SimpleArg)
+    SimpleArg = stripFloatLiteralFraction(Result, *Arg);
+
+  if (SimpleArg) {
     diag(MatchedCall->getBeginLoc(),
          (llvm::Twine("use the integer version of absl::") +
           MatchedCall->getDirectCallee()->getName())
              .str())
-        << FixItHint::CreateReplacement(
+        << FixItHint::CreateReplacement(Arg->getSourceRange(), *SimpleArg);
-               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;
-    }
   }
 }
 

clang-tools-extra/clang-tidy/abseil/DurationFactoryScaleCheck.cpp

@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "DurationFactoryScaleCheck.h"
+#include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/FixIt.h"
@@ -18,20 +19,6 @@ namespace clang {
 namespace tidy {
 namespace abseil {
 
-namespace {
-
-// Potential scales of our inputs.
-enum class DurationScale {
-  Hours,
-  Minutes,
-  Seconds,
-  Milliseconds,
-  Microseconds,
-  Nanoseconds,
-};
-
-} // namespace
-
 // 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`.
@@ -129,39 +116,14 @@ static llvm::Optional<DurationScale> GetNewScale(DurationScale OldScale,
   return llvm::None;
 }
 
-// Given a `Scale`, return the appropriate factory function call for
-// constructing a `Duration` for that scale.
-static llvm::StringRef 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";
-  }
-  llvm_unreachable("unknown scaling factor");
-}
-
 void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       callExpr(
-          callee(functionDecl(
+          callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
-                     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"),
+                  integerLiteral(equals(0)), floatLiteral(equals(0.0)),
-                  floatLiteral(equals(0.0)).bind("zero"),
                   binaryOperator(hasOperatorName("*"),
                                  hasEitherOperand(ignoringImpCasts(
                                      anyOf(integerLiteral(), floatLiteral()))))
@@ -185,7 +147,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
     return;
 
   // We first handle the cases of literal zero (both float and integer).
-  if (Result.Nodes.getNodeAs<Stmt>("zero")) {
+  if (IsLiteralZero(Result, *Arg)) {
     diag(Call->getBeginLoc(),
          "use ZeroDuration() for zero-length time intervals")
         << FixItHint::CreateReplacement(Call->getSourceRange(),
@@ -244,7 +206,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
       diag(Call->getBeginLoc(), "internal duration scaling can be removed")
           << FixItHint::CreateReplacement(
                  Call->getSourceRange(),
-                 (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+                 (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
                   tooling::fixit::getText(*Remainder, *Result.Context) + ")")
                      .str());
     }
@@ -257,7 +219,7 @@ void DurationFactoryScaleCheck::check(const MatchFinder::MatchResult &Result) {
     diag(Call->getBeginLoc(), "internal duration scaling can be removed")
         << FixItHint::CreateReplacement(
                Call->getSourceRange(),
-               (llvm::Twine(GetFactoryForScale(*NewScale)) + "(" +
+               (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
                 tooling::fixit::getText(*Remainder, *Result.Context) + ")")
                    .str());
   }

clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp

@@ -0,0 +1,109 @@
+//===--- DurationRewriter.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 "DurationRewriter.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 the factory function name for a given `Scale`.
+llvm::StringRef 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";
+  }
+  llvm_unreachable("unknown scaling factor");
+}
+
+/// Returns `true` if `Node` is a value which evaluates to a literal `0`.
+bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) {
+  return selectFirst<const clang::Expr>(
+             "val",
+             match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),
+                                               floatLiteral(equals(0.0)))))
+                       .bind("val"),
+                   Node, *Result.Context)) != nullptr;
+}
+
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+               const Expr &Node) {
+  if (const Expr *MaybeCastArg = selectFirst<const Expr>(
+          "cast_arg",
+          match(expr(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"))))),
+                Node, *Result.Context)))
+    return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str();
+
+  return llvm::None;
+}
+
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const MatchFinder::MatchResult &Result,
+                          const Expr &Node) {
+  if (const auto *LitFloat = llvm::dyn_cast<FloatingLiteral>(&Node))
+    // Attempt to simplify a `Duration` factory call with a literal argument.
+    if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat))
+      return IntValue->toString(/*radix=*/10);
+
+  return llvm::None;
+}
+
+std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
+                                       const Expr &Node) {
+  // Check for an explicit cast to `float` or `double`.
+  if (llvm::Optional<std::string> MaybeArg = stripFloatCast(Result, Node))
+    return *MaybeArg;
+
+  // Check for floats without fractional components.
+  if (llvm::Optional<std::string> MaybeArg =
+          stripFloatLiteralFraction(Result, Node))
+    return *MaybeArg;
+
+  // We couldn't simplify any further, so return the argument text.
+  return tooling::fixit::getText(Node, *Result.Context).str();
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/abseil/DurationRewriter.h

@@ -0,0 +1,86 @@
+//===--- DurationRewriter.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_DURATIONREWRITER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONREWRITER_H
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include <cinttypes>
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Duration factory and conversion scales
+enum class DurationScale : std::int8_t {
+  Hours,
+  Minutes,
+  Seconds,
+  Milliseconds,
+  Microseconds,
+  Nanoseconds,
+};
+
+/// Given a `Scale`, return the appropriate factory function call for
+/// constructing a `Duration` for that scale.
+llvm::StringRef getFactoryForScale(DurationScale Scale);
+
+// Determine if `Node` represents a literal floating point or integral zero.
+bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
+                   const Expr &Node);
+
+/// Possibly strip a floating point cast expression.
+///
+/// If `Node` represents an explicit cast to a floating point type, return
+/// the textual context of the cast argument, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
+               const Expr &Node);
+
+/// Possibly remove the fractional part of a floating point literal.
+///
+/// If `Node` represents a floating point literal with a zero fractional part,
+/// return the textual context of the integral part, otherwise `None`.
+llvm::Optional<std::string>
+stripFloatLiteralFraction(const ast_matchers::MatchFinder::MatchResult &Result,
+                          const Expr &Node);
+
+/// Possibly further simplify a duration factory function's argument, without
+/// changing the scale of the factory function. Return that simplification or
+/// the text of the argument if no simplification is possible.
+std::string
+simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result,
+                           const Expr &Node);
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+                     DurationConversionFunction) {
+  using namespace clang::ast_matchers;
+  return functionDecl(
+      hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+                 "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
+                 "::absl::ToDoubleMicroseconds", "::absl::ToDoubleNanoseconds",
+                 "::absl::ToInt64Hours", "::absl::ToInt64Minutes",
+                 "::absl::ToInt64Seconds", "::absl::ToInt64Milliseconds",
+                 "::absl::ToInt64Microseconds", "::absl::ToInt64Nanoseconds"));
+}
+
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+                     DurationFactoryFunction) {
+  using namespace clang::ast_matchers;
+  return functionDecl(hasAnyName("::absl::Nanoseconds", "::absl::Microseconds",
+                                 "::absl::Milliseconds", "::absl::Seconds",
+                                 "::absl::Minutes", "::absl::Hours"));
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -67,6 +67,12 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-duration-comparison
+  <clang-tidy/checks/abseil-duration-comparison>` check.
+
+  Checks for comparisons which should be done in the ``absl::Duration`` domain
+  instead of the float of integer domains.
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 

clang-tools-extra/docs/clang-tidy/checks/abseil-duration-comparison.rst

@@ -0,0 +1,33 @@
+.. title:: clang-tidy - abseil-duration-comparison
+
+abseil-duration-comparison
+==========================
+
+Checks for comparisons which should be in the ``absl::Duration`` domain instead
+of the floating point or integer domains.
+
+N.B.: In cases where a ``Duration`` was being converted to an integer and then
+compared against a floating-point value, truncation during the ``Duration``
+conversion might yield a different result. In practice this is very rare, and
+still indicates a bug which should be fixed.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Comparison in the floating point domain
+  double x;
+  absl::Duration d;
+  if (x < absl::ToDoubleSeconds(d)) ...
+
+  // Suggested - Compare in the absl::Duration domain instead
+  if (absl::Seconds(x) < d) ...
+
+
+  // Original - Comparison in the integer domain
+  int x;
+  absl::Duration d;
+  if (x < absl::ToInt64Microseconds(d)) ...
+
+  // Suggested - Compare in the absl::Duration domain instead
+  if (absl::Microseconds(x) < d) ...

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -4,6 +4,7 @@ Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-duration-comparison
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale

clang-tools-extra/test/clang-tidy/abseil-duration-comparison.cpp

@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+class Time{};
+
+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
+
+using int64_t = long long int;
+
+double ToDoubleHours(Duration d);
+double ToDoubleMinutes(Duration d);
+double ToDoubleSeconds(Duration d);
+double ToDoubleMilliseconds(Duration d);
+double ToDoubleMicroseconds(Duration d);
+double ToDoubleNanoseconds(Duration d);
+int64_t ToInt64Hours(Duration d);
+int64_t ToInt64Minutes(Duration d);
+int64_t ToInt64Seconds(Duration d);
+int64_t ToInt64Milliseconds(Duration d);
+int64_t ToInt64Microseconds(Duration d);
+int64_t ToInt64Nanoseconds(Duration d);
+
+// Relational Operators
+constexpr bool operator<(Duration lhs, Duration rhs);
+constexpr bool operator>(Duration lhs, Duration rhs);
+constexpr bool operator>=(Duration lhs, Duration rhs);
+constexpr bool operator<=(Duration lhs, Duration rhs);
+constexpr bool operator==(Duration lhs, Duration rhs);
+constexpr bool operator!=(Duration lhs, Duration rhs);
+
+// Additive Operators
+inline Time operator+(Time lhs, Duration rhs);
+inline Time operator+(Duration lhs, Time rhs);
+inline Time operator-(Time lhs, Duration rhs);
+inline Duration operator-(Time lhs, Time rhs);
+
+}  // namespace absl
+
+void f() {
+  double x;
+  absl::Duration d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 == absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) >= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 >= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) > x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > absl::Seconds(x);
+
+  // Comparison against zero
+  b = absl::ToDoubleSeconds(d1) < 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::ZeroDuration();
+  b = absl::ToDoubleSeconds(d1) < 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::ZeroDuration();
+
+  // Scales other than Seconds
+  b = x > absl::ToDoubleMicroseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Microseconds(x) > d1;
+  b = x >= absl::ToDoubleMilliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+  b = x == absl::ToDoubleNanoseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+  b = x <= absl::ToDoubleMinutes(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToDoubleHours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Hours(x) < d1;
+
+  // Integer comparisons
+  b = x > absl::ToInt64Microseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Microseconds(x) > d1;
+  b = x >= absl::ToInt64Milliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds(x) >= d1;
+  b = x == absl::ToInt64Nanoseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Nanoseconds(x) == d1;
+  b = x == absl::ToInt64Seconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToInt64Minutes(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToInt64Hours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Hours(x) < d1;
+
+  // Other abseil-duration checks folded into this one
+  b = static_cast<double>(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = double(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = float(5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = ((double)5) > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+  b = 5.0 > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(5) > d1;
+
+  // A long expression
+  bool some_condition;
+  int very_very_very_very_long_variable_name;
+  absl::Duration SomeDuration;
+  if (some_condition && very_very_very_very_long_variable_name
+     < absl::ToDoubleSeconds(SomeDuration)) {
+  // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: if (some_condition && absl::Seconds(very_very_very_very_long_variable_name) < SomeDuration) {
+    return;
+  }
+
+  // A complex expression
+  int y;
+  b = (y + 5) * 10 > absl::ToDoubleMilliseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
+
+  // These should not match
+  b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToDoubleSeconds(x)
+  b = 5.0 > TODOUBLE(d1);
+#undef TODOUBLE
+#define THIRTY 30.0
+  b = THIRTY > absl::ToDoubleSeconds(d1);
+#undef THIRTY
+}