[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: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