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