[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/DurationDivisionCheck.h:23
 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html
 
 class DurationDivisionCheck : public ClangTidyCheck {
----------------
JonasToth wrote:
> hwright wrote:
> > JonasToth wrote:
> > > I think that blank line could be removed, and it seems the comment is not ///, could you take a look at it too?
> > > Touching this file is probably better to do in another patch anyway.
> > Agreed.  I think this snuck into the patch; I'll remove it.
> > 
> > (It would be good to just `clang-format` everything in this directory in a separate patch.)
> > 
> > The comment issue with `///` seems to be a common problem; is `clang-tidy/add_new_check.py` not generating correct code?
> add_new_check does ///, maybe IDE settings or so removed these? Maybe someone created everything manually, dunno.
> 
> Doing the clang-format is ok, doesn't need review either (but plz run the test before committing to master).
I don't think I yet have the commit bit...so somebody else wouldn't need to directly commit. :)


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


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

https://reviews.llvm.org/D54737