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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
