JonasToth added a comment. LG from my side, only the style nits left. other reviewers are invited to take a look too :)
================ Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:22 + +// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`), +// return its `DurationScale`, or `None` if a match is not found. ---------------- Please use triple / for the function comment, for doxygen and consistency with documentation ================ 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); ---------------- non-existence? Not sure about english, but i thought english does it that way ================ Comment at: clang-tidy/abseil/DurationComparisonCheck.h:20 +/// Prefer comparison in the absl::Duration domain instead of the numeric +// domain. +/// ---------------- Missing / ================ Comment at: clang-tidy/abseil/DurationDivisionCheck.h:23 // http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-division.html class DurationDivisionCheck : public ClangTidyCheck { ---------------- 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. ================ Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61 + + if (SimpleArg) { diag(MatchedCall->getBeginLoc(), ---------------- 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::-...`) ================ Comment at: clang-tidy/abseil/DurationRewriter.cpp:19 + +// Returns an integer if the fractional part of a `FloatingLiteral` is `0`. +static llvm::Optional<llvm::APSInt> ---------------- Comment ================ Comment at: clang-tidy/abseil/DurationRewriter.cpp:75 + return tooling::fixit::getText(*MaybeCastArg, *Result.Context).str(); + } + ---------------- you can ellide the braces ================ Comment at: clang-tidy/abseil/DurationRewriter.cpp:85 + // Attempt to simplify a `Duration` factory call with a literal argument. + if (llvm::Optional<llvm::APSInt> IntValue = truncateIfIntegral(*LitFloat)) { + return IntValue->toString(/*radix=*/10); ---------------- you can ellide the braces here ================ Comment at: clang-tidy/abseil/DurationRewriter.h:21 + +// Duration factory and conversion scales +enum class DurationScale : std::int8_t { ---------------- Same comment things in this file (///) ================ Comment at: clang-tidy/abseil/DurationRewriter.h:56 +// Possibly further simplify a duration factory function's argument, without +// changing the scale of the factory function. Return that simplification or +// the text of the argument if no simplification is possible. ---------------- Double space 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
