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

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked 4 inline comments as done.
hwright added inline comments.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
     diag(MatchedCall->getBeginLoc(),
----------------
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?


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

https://reviews.llvm.org/D54737