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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
