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

[Jonas Toth via Phabricator via cfe-commits]

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