[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:23
+  auto Matcher =
+      binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+                           hasOperatorName("=="), hasOperatorName("<="),
----------------
`DurationComparisonCheck.cpp` has a very similar matcher pattern.

Maybe pull out a common matcher for this? E.g. `comparisonOperatorWithCallee(...)`?



================
Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:42
+
+  // 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
----------------
Move this comment closer the replacement logic below?


================
Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:46
+  // if nothing needs to be done.
+  if (!isNotInMacro(Result, Binop->getLHS()) ||
+      !isNotInMacro(Result, Binop->getRHS()))
----------------
nit: double negation is a bit hard to read. maybe `!(isNotInMacro(LSH) && isNotInMacro(RHS))`? Ideally, the helper should be `isInMacro` instead of `isNotInMacro`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58977