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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth 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 {
----------------
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).


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


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

https://reviews.llvm.org/D54737