[PATCH] D55784: [clang-tidy] Update abseil-duration-comparison to handle macro arguments

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked 2 inline comments as done.
hwright added inline comments.


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:146
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
+  int a3 = VALUE_IF(1, d1, Double);
----------------
aaron.ballman wrote:
> hwright wrote:
> > aaron.ballman wrote:
> > > What if this is changed to:
> > > ```
> > > #define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
> > > ```
> > This also doesn't transform.
> Fantastic, thank you! I wouldn't expect a fix-it there due to the macro shenanigans, but I'm surprised it does not diagnose given that it expands to basically the same thing as `a` and `a2` above: https://godbolt.org/z/D1MvGX (ignore the errors but look at the macro expansion from the diagnostics).
> 
> Granted, this situation is uncommon enough, so feel free to add a FIXME comment to the `a4` test case to say that it probably should be diagnosed without a fix-it hint (assuming you agree).
I actually prefer not to diagnose here, mostly because I'm not sure what value we'd actually give without a fix-it hint.  "You are holding it wrong, but we can't tell you how to hold it right."

Ultimately, though, I think you are right that the number of cases this occurs are vanishingly small, and I'd expect folks doing these kinds of craziness to know what they are doing (and take responsibility thereof).  Perhaps that's optimistic.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55784/new/

https://reviews.llvm.org/D55784