Page MenuHomePhabricator

[clang-tidy] Update abseil-duration-comparison to handle macro arguments
ClosedPublic

Authored by hwright on Dec 17 2018, 12:23 PM.

Details

Summary

This change relaxes the requirements on the utility rewriteExprFromNumberToDuration function, so that the abseil-duration-comparison check transform expressions which are arguments of macros.

Diff Detail

Repository
rL LLVM

Event Timeline

hwright created this revision.Dec 17 2018, 12:23 PM
hwright set the repository for this revision to rCTE Clang Tools Extra.
hwright added a project: Restricted Project.
aaron.ballman added inline comments.Tue, Dec 18, 5:40 AM
clang-tidy/abseil/DurationComparisonCheck.cpp
25 ↗(On Diff #178511)

expr doesn't follow the usual naming conventions, I'd go with E (and fix the comment to match).

29 ↗(On Diff #178511)

loc -> Loc

test/clang-tidy/abseil-duration-comparison.cpp
131 ↗(On Diff #178511)

Can you add another example that has one more level of macro arg expansion? e.g.,

#define VALUE_IF_2(e) (e)
#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
int a = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1));

Which makes me wonder -- is this transformation valid in all cases? For instance, how do the fixes look with this abomination?

#define VALUE_IF_2(e) (e)
#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
int a = VALUE_IF(1, d1, Double);

I would expect these shenanigans to be vanishingly rare, so if this turns out to be a bad FIXIT you may want to document it as a known deficiency if you don't feel like handling such a case.

hwright updated this revision to Diff 178680.Tue, Dec 18, 8:03 AM
hwright marked 4 inline comments as done.

Update tests

test/clang-tidy/abseil-duration-comparison.cpp
131 ↗(On Diff #178511)

I've added both these as test cases.

The first one works as expected, and I'm pleasantly surprised to see that the second one does as well (which is to say that it doesn't transform).

aaron.ballman added inline comments.Tue, Dec 18, 8:23 AM
test/clang-tidy/abseil-duration-comparison.cpp
131 ↗(On Diff #178511)

Great, thank you!

146 ↗(On Diff #178680)

What if this is changed to:

#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
hwright updated this revision to Diff 178733.Tue, Dec 18, 10:42 AM
hwright marked 3 inline comments as done.

Another test.

@aaron.ballman I am both grateful and sad that I don't possess the same macro creativity as you do. :)

(I'm also sad that the various clang APIs aren't as well documented as I hoped. The current solution works well, but it took a while and a bit of consultation with some other llvm'ers to get it right.)

test/clang-tidy/abseil-duration-comparison.cpp
146 ↗(On Diff #178680)

This also doesn't transform.

aaron.ballman accepted this revision.Wed, Dec 19, 6:14 AM

@aaron.ballman I am both grateful and sad that I don't possess the same macro creativity as you do. :)

LOL

(I'm also sad that the various clang APIs aren't as well documented as I hoped. The current solution works well, but it took a while and a bit of consultation with some other llvm'ers to get it right.)

As you dig into APIs like that, feel free to propose or commit new documentation that helps clarify things -- more/better docs are always appreciated!

LGTM with a minor commenting nit.

test/clang-tidy/abseil-duration-comparison.cpp
146 ↗(On Diff #178680)

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).

This revision is now accepted and ready to land.Wed, Dec 19, 6:14 AM
hwright marked 2 inline comments as done.Wed, Dec 19, 8:01 AM
hwright added inline comments.
test/clang-tidy/abseil-duration-comparison.cpp
146 ↗(On Diff #178680)

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.

This revision was automatically updated to reflect the committed changes.
hwright marked an inline comment as done.