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