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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
