[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

Only the test and your opinion on the `Optional` thing, If you want to keep it that way its fine.

LGTM afterwards :)



================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
     diag(MatchedCall->getBeginLoc(),
----------------
hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > The diagnostic is not printed if for some reason the fixit was not creatable. I think that the warning could still be emitted (`auto Diag = diag(...); if (Fix) Diag << Fixit::-...`)
> > > > > I'm not sure under which conditions you'd expect this to be an issue.  Could you give me an example?
> > > > > 
> > > > > My expectation is that if we don't get a value back in `SimpleArg`, we don't have anything to change, so there wouldn't be a warning to emit.
> > > > I don't expect this to fail, failure there would mean a bug i guess. Having bugs is somewhat expected :)
> > > > And that would be our way to find the bug, because some user reports that there is no transformation done, that is my motivation behind that.
> > > > 
> > > > The warning itself should be correct, otherwise the matcher does not work, right? This would just be precaution.
> > > I guess what I'm saying is that I'm not sure what kind of warning we'd give if we weren't able to offer a fix.  The optionality here means that we found a result which was already as simple as we can make it, so there's no reason to bother the user.
> > Lets say the result comes out of arithmetic and then there is a comparison (would be good test btw :)), the warning is still valueable, even if the transformation fails for some reason. The transformation could fail as well, because macros interfere or so. Past experience tells me, there are enough language constructs to break everything in weird combinations :)
> I can buy that, but I'm still having trouble seeing the specifics.  Do you have a concrete example?
No i can't think of one right now. Its more a general argument, that the code-layout logically allows that only the transformation fails, even if the detection succeeds. No strong opinion, but preference :)


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:148
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToInt64Hours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
----------------
please add a test where the `int` part is result of a complex arithmetic expression.


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

https://reviews.llvm.org/D54737