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

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.


================
Comment at: clang-tidy/abseil/DurationAdditionCheck.cpp:22
+void DurationAdditionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("+"),
----------------
JonasToth wrote:
> 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?
I think that most of it already has been factored out (e.g., `rewriteExprFromNumberToDuration` and `getScaleForTimeInverse`, etc) . The actual meat here is pretty light: the matcher, and then the diagnostic generation.

A scaling change may also follow.  Our experience has been that scaling isn't quite straight forward, as the scaling factor may have semantic meaning which changes the result, but which isn't captured by the type system.  Probably not a design discussion to have here, but suffice it to say that I don't know if this is yet settled.


================
Comment at: clang-tidy/abseil/DurationAdditionCheck.cpp:50
+    diag(Binop->getBeginLoc(), "perform addition in the duration domain")
+        << FixItHint::CreateReplacement(
+               Binop->getSourceRange(),
----------------
JonasToth wrote:
> The only difference between the two `diag` seems to be the `FixItHint`. I would rather refactor the diag out of the `if`.
This doesn't really de-dupe very much, since the bulk of the work here is creating the `FixItHint`.  Done.


================
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")
----------------
JonasToth wrote:
> What happens with paren-casts, are they ignored as well? (See one comment in the test-cases)
Now they are.


================
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]
----------------
JonasToth wrote:
> Call is on the right side, what happens for `3 + (abs::ToUnixHours(t));`
Added this test, and updated to ignore paren casts.


================
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]
----------------
JonasToth wrote:
> 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.
Semantically, this is possible, but wouldn't fall under this transform, since it would require changing the type of `i`.


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

https://reviews.llvm.org/D57185