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

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:69
+  // We know our map contains all the Scale values, so we can skip the
+  // nonexistence check.
+  auto InverseIter = InverseMap.find(Scale);
----------------
JonasToth wrote:
> non-existence? Not sure about english, but i thought english does it that way
https://www.merriam-webster.com/dictionary/nonexistence tells me the hyphen isn't required.


================
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:
> 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?


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


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

https://reviews.llvm.org/D54737