[PATCH] D58137: [clang-tidy] Add the abseil-time-subtraction check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:92
+                       DurationScale Scale, const Expr &Node) {
+  const llvm::StringRef &InverseFunction = getTimeInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
----------------
nit: stringref should be used as value all the time (as its a pointer itself).


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:65
+    llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(TimeInverse);
+    if (!Scale)
+      continue;
----------------
wouldn't that be rather a bug condition? I think `assert` is better in that case


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:72
+
+    // Match the cases where we know that the result is a Duration and the first
+    // argument is a Time.  Just knowing the type of the first operand is not
----------------
please highlight the types with ''


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:97
+void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  std::string inverse_name =
----------------
Could you please split this function up into smaller ones. There are three or four distinct cases that are easier to comprehend in isolation.


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:98
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  std::string inverse_name =
+      Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString();
----------------
nit: `InverseName`


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.cpp:108
+    // We're working with the first case of matcher, and need to replace the
+    // entire Duration factory call.  (Which also means being careful about
+    // our order-of-operations and optionally putting in some parenthesis.
----------------
please elimate the multiple blanks


================
Comment at: clang-tidy/abseil/TimeSubtractionCheck.h:19
+/// Finds and fixes `absl::Time` subtraction expressions to do subtraction
+/// in the Time domain instead of the numeric domain.
+///
----------------
nit: 'Time' domain


================
Comment at: test/clang-tidy/abseil-time-subtraction.cpp:12
+
+  d = absl::Hours(absl::ToUnixHours(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
----------------
please add tests where `x` itself is a calculation with different precedence of its operators (multiplication, addition) to ensure these cases are transformed properly as well.


================
Comment at: test/clang-tidy/abseil-time-subtraction.cpp:78
+  // CHECK-FIXES: return absl::FromUnixSeconds(x) - t;
+}
----------------
please add tests for templates and macros.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58137/new/

https://reviews.llvm.org/D58137