JonasToth added inline comments.
================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+ // Macros are ignored.
+ if (Arg->getBeginLoc().isMacroID())
+ return;
----------------
hwright wrote:
> 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.
Ok, I misunderstood the code then :)
================
Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+ absl::Duration d;
+
----------------
hwright wrote:
> 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.
I am happy with that. I agree that parsing should deal with it fine and your
code seems to do it fine as well. My experience is, that sometimes there are
still surprises :)
https://reviews.llvm.org/D53339
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
