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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:34
+    llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+    ApInt = static_cast<unsigned long long>(value);
+    if (is_negative)
----------------
Wouldn't it make more sense to use `std::uint64_t` instead to correspond to the line above? And where does the signedness difference come from? (`/*isUnsigned=*/false`)


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+  // Don't try and replace things inside of macro definitions.
+  if (!clang::Lexer::makeFileCharRange(
+           clang::CharSourceRange::getCharRange(MatchedCall->getSourceRange()),
----------------
That is worth a helper function taking a `SourceRange` as argument.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+  // Macros are ignored.
+  if (Arg->getBeginLoc().isMacroID())
+    return;
----------------
maybe `assert` instead, as your comment above suggests that macros are already filtered out?


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:72
+
+  // Check for static casts to float
+  if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
----------------
missing full stop.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:84
+
+  // Check for floats without fractional components
+  if (const auto *LitFloat =
----------------
missing full stop


================
Comment at: docs/clang-tidy/checks/list.rst:12
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
----------------
spurious change


================
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+  absl::Duration d;
+
----------------
Could you also provide test cases with hexadecimal floating literals, which are C++17? The thousand-separators could be checked as well (dunno the correct term right now, but the `1'000'000` feature).
Please add test cases for negative literals, too.


https://reviews.llvm.org/D53339