[PATCH] D57185: [clang-tidy] Add the abseil-duration-addition check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationAdditionCheck.cpp:22
+void DurationAdditionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("+"),
----------------
This whole file is structurally similar to the `DurationSubtractionCheck.cpp` equivalent (which is expected :)), maybe some parts could be refactored out?
Will there be a check for duration scaling or the like?


================
Comment at: clang-tidy/abseil/DurationAdditionCheck.cpp:50
+    diag(Binop->getBeginLoc(), "perform addition in the duration domain")
+        << FixItHint::CreateReplacement(
+               Binop->getSourceRange(),
----------------
The only difference between the two `diag` seems to be the `FixItHint`. I would rather refactor the diag out of the `if`.


================
Comment at: clang-tidy/abseil/DurationAdditionCheck.cpp:60
+  } else {
+    assert(Call == Binop->getRHS()->IgnoreImpCast() && "Call should be found on the RHS");
+    diag(Binop->getBeginLoc(), "perform addition in the duration domain")
----------------
What happens with paren-casts, are they ignored as well? (See one comment in the test-cases)


================
Comment at: test/clang-tidy/abseil-duration-addition.cpp:28
+
+  i = 3 + absl::ToUnixHours(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition]
----------------
Call is on the right side, what happens for `3 + (abs::ToUnixHours(t));`


================
Comment at: test/clang-tidy/abseil-duration-addition.cpp:48
+  // Undoing inverse conversions
+  i = absl::ToUnixMicros(t) + absl::ToInt64Microseconds(absl::Seconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform addition in the duration domain [abseil-duration-addition]
----------------
Is something like this
```
i += absl::ToInt64MicroSeconds(absl::Seconds(1));
```
possible (syntactically and semantically)?
The compound operators are currently not covered by this (and the subtraction-check), but in this case it looks like a possible use-case.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57185