[clang-tidy] Add the abseil-time-compare check

This is an analog of the abseil-duration-comparison check, but for the
absl::Time domain. It has a similar implementation and tests.

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

llvm-svn: 355835

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

@@ -23,6 +23,7 @@
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
+#include "TimeComparisonCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
@@ -60,6 +61,8 @@ class AbseilModule : public ClangTidyModule {
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
+    CheckFactories.registerCheck<TimeComparisonCheck>(
+        "abseil-time-comparison");
     CheckFactories.registerCheck<TimeSubtractionCheck>(
         "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(

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

@@ -17,6 +17,7 @@ add_clang_library(clangTidyAbseilModule
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
+  TimeComparisonCheck.cpp
   TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 

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

@@ -19,14 +19,10 @@ namespace tidy {
 namespace abseil {
 
 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");
+  auto Matcher = expr(comparisonOperatorWithCallee(functionDecl(
+                          functionDecl(DurationConversionFunction())
+                              .bind("function_decl"))))
+                     .bind("binop");
 
   Finder->addMatcher(Matcher, this);
 }

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

@@ -124,6 +124,16 @@ AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
       "::absl::ToUnixMillis", "::absl::ToUnixMicros", "::absl::ToUnixNanos"));
 }
 
+AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher<Stmt>,
+                       comparisonOperatorWithCallee,
+                       ast_matchers::internal::Matcher<Decl>, funcDecl) {
+  using namespace clang::ast_matchers;
+  return binaryOperator(
+      anyOf(hasOperatorName(">"), hasOperatorName(">="), hasOperatorName("=="),
+            hasOperatorName("<="), hasOperatorName("<")),
+      hasEitherOperand(ignoringImpCasts(callExpr(callee(funcDecl)))));
+}
+
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

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

@@ -0,0 +1,61 @@
+//===--- TimeComparisonCheck.cpp - clang-tidy
+//--------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TimeComparisonCheck.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 {
+
+void TimeComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher =
+      expr(comparisonOperatorWithCallee(functionDecl(
+               functionDecl(TimeConversionFunction()).bind("function_decl"))))
+          .bind("binop");
+
+  Finder->addMatcher(Matcher, this);
+}
+
+void TimeComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+  llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(
+      Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+  if (!Scale)
+    return;
+
+  if (isInMacro(Result, Binop->getLHS()) || isInMacro(Result, Binop->getRHS()))
+    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.
+  std::string LhsReplacement =
+      rewriteExprFromNumberToTime(Result, *Scale, Binop->getLHS());
+  std::string RhsReplacement =
+      rewriteExprFromNumberToTime(Result, *Scale, Binop->getRHS());
+
+  diag(Binop->getBeginLoc(), "perform comparison in the time domain")
+      << FixItHint::CreateReplacement(Binop->getSourceRange(),
+                                      (llvm::Twine(LhsReplacement) + " " +
+                                       Binop->getOpcodeStr() + " " +
+                                       RhsReplacement)
+                                          .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,35 @@
+//===--- TimeComparisonCheck.h - clang-tidy ---------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the `absl::Time` domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-time-comparison.html
+class TimeComparisonCheck : public ClangTidyCheck {
+public:
+  TimeComparisonCheck(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_TIMECOMPARECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -85,6 +85,12 @@ Improvements to clang-tidy
   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
+- New :doc:`abseil-time-comparison
+  <clang-tidy/checks/abseil-time-comparison>` check.
+
+  Prefer comparisons in the ``absl::Time`` domain instead of the integer
+  domain.
+
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 

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

@@ -0,0 +1,23 @@
+.. title:: clang-tidy - abseil-time-comparison
+
+abseil-time-comparison
+======================
+
+Prefer comparisons in the ``absl::Time`` domain instead of the integer domain.
+
+N.B.: In cases where an ``absl::Time`` is being converted to an integer,
+alignment may occur. If the comparison depends on this alingment, doing the
+comparison in the ``absl::Time`` domain may 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 integer domain
+  int x;
+  absl::Time t;
+  if (x < absl::ToUnixSeconds(t)) ...
+
+  // Suggested - Compare in the absl::Time domain instead
+  if (absl::FromUnixSeconds(x) < t) ...

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

@@ -18,6 +18,7 @@ Clang-Tidy Checks
    abseil-redundant-strcat-calls
    abseil-str-cat-append
    abseil-string-find-startswith
+   abseil-time-comparison
    abseil-time-subtraction
    abseil-upgrade-duration-conversions
    android-cloexec-accept

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

@@ -0,0 +1,129 @@
+// RUN: %check_clang_tidy %s abseil-time-comparison %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) > t1;
+  b = x >= absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) >= t1;
+  b = x == absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) == t1;
+  b = x <= absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) <= t1;
+  b = x < absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) < t1;
+  b = x == absl::ToUnixSeconds(t1 - d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) == t1 - d2;
+  b = absl::ToUnixSeconds(t1) > absl::ToUnixSeconds(t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 > t2;
+
+  // Check against the LHS
+  b = absl::ToUnixSeconds(t1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 <= absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 == absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) >= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 >= absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) > x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 > absl::FromUnixSeconds(x);
+
+  // Comparison against zero
+  b = absl::ToUnixSeconds(t1) < 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::UnixEpoch();
+  b = absl::ToUnixSeconds(t1) < 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::UnixEpoch();
+
+  // Scales other than Seconds
+  b = x > absl::ToUnixMicros(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMicros(x) > t1;
+  b = x >= absl::ToUnixMillis(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMillis(x) >= t1;
+  b = x == absl::ToUnixNanos(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixNanos(x) == t1;
+  b = x <= absl::ToUnixMinutes(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMinutes(x) <= t1;
+  b = x < absl::ToUnixHours(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixHours(x) < t1;
+
+  // A long expression
+  bool some_condition;
+  int very_very_very_very_long_variable_name;
+  absl::Time SomeTime;
+  if (some_condition && very_very_very_very_long_variable_name
+     < absl::ToUnixSeconds(SomeTime)) {
+  // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: if (some_condition && absl::FromUnixSeconds(very_very_very_very_long_variable_name) < SomeTime) {
+    return;
+  }
+
+  // A complex expression
+  int y;
+  b = (y + 5) * 10 > absl::ToUnixMillis(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMillis((y + 5) * 10) > t1;
+
+  // We should still transform the expression inside this macro invocation
+#define VALUE_IF(v, e) v ? (e) : 0
+  int a = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
+  int a2 = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
+  int a3 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
+  int a4 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+  // These should not match
+  b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToUnixSeconds(x)
+  b = 5.0 > TODOUBLE(t1);
+#undef TODOUBLE
+#define THIRTY 30.0
+  b = THIRTY > absl::ToUnixSeconds(t1);
+#undef THIRTY
+}