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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 18 05:40:21 PST 2018

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.

  rCTE Clang Tools Extra



More information about the cfe-commits mailing list