[PATCH] D53339: Add the abseil-duration-factory-float clang-tidy check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

LG In general, but see a few comments inline.



================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:25
+llvm::Optional<llvm::APSInt>
+TruncateIfIntegral(const FloatingLiteral &FloatLiteral) {
+  double value = FloatLiteral.getValueAsApproximateDouble();
----------------
Please use the LLVM style (`functionName`). See http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:27
+  double value = FloatLiteral.getValueAsApproximateDouble();
+  if (std::fmod(value, 1) == 0) {
+    bool is_negative = false;
----------------
Probably doesn't matter much, but would `std::modf` be more appropriate in this context?


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:65-66
+  // Don't try and replace things inside of macro definitions.
+  if (MatchedCall->getBeginLoc().isMacroID() ||
+      MatchedCall->getEndLoc().isMacroID())
+    return;
----------------
Lexer::makeFileCharRange may be a better (and less strict) way to test whether we can safely replace a range.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:87
+  // Check for floats without fractional components
+  if (const FloatingLiteral *LitFloat =
+          Result.Nodes.getNodeAs<FloatingLiteral>("float_literal")) {
----------------
`const auto *` will be as readable and less verbose.


================
Comment at: docs/ReleaseNotes.rst:73
+
+  FIXME: add release notes.
+
----------------
Please remove the FIXME


https://reviews.llvm.org/D53339