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

[Hyrum Wright via Phabricator via cfe-commits]

hwright 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)
----------------
JonasToth wrote:
> 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`)
I don't remember where the signedness difference came from, so I've made this `std::int64_t`.


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

In the first case, we want to be sure we aren't replacing cases inside of a macro, such as:
```
#define SECONDS(x) absl::Seconds(x)
SECONDS(1.0)
```

In this one, we want to avoid changing the argument if it is itself a macro:
```
#define VAL 1.0
absl::Seconds(VAL);
```

So it is a separate check, not just a re-assertion of the first one.


================
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+  absl::Duration d;
+
----------------
JonasToth wrote:
> 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.
Added the hexadecimal floating literal tests.

The testing infrastructure wouldn't build the test source with `3'000` as a literal argument.  (There's also an argument that by the time we get to the AST, that distinction is gone anyway and this test isn't the place to check comprehensive literal parsing.)

I've also added a negative literal test, to illustrate that we don't yet handle that case (which is in practice pretty rare).  I'd rather add it in a separate change.


https://reviews.llvm.org/D53339