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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:25
+static bool IsValidMacro(const MatchFinder::MatchResult &Result,
+                         const Expr *expr) {
+  if (!expr->getBeginLoc().isMacroID())
----------------
`expr` doesn't follow the usual naming conventions, I'd go with `E` (and fix the comment to match).


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:29
+
+  SourceLocation loc = expr->getBeginLoc();
+  // We want to get closer towards the initial macro typed into the source only
----------------
loc -> Loc


================
Comment at: test/clang-tidy/abseil-duration-comparison.cpp:131
+  // We should still transform the expression inside this macro invocation
+#define VALUE_IF(v, e) v ? (e) : 0
+  int a = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1));
----------------
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.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55784