[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example).
WDYT about those? Are they important, should we support producing warnings in those cases to let the users know things might get broken?



================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+  const SourceManager &SM = *Result.SourceManager;
----------------
Could we add unit tests for this particular function?

Interesting cases (`[[` and `]]` mark the start and end of a range):
```
#define FOO(a) a+a;
#define BAR 10+

// change part of a macro argument
int a = FOO([[10]] + 10);

// change the whole macro expansion
int b = [[FOO(10+10)]];

// Try to change 10 inside 'BAR', but not '+'.
// Should this fail? Should we give a warning?
int c = BAR 3; 

// Try changing the lhs (10) of a binary expr, but not rhs.
// Is that allowed? Should we give a warning?
int d = FOO(10);
```


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:99
+
+  std::pair<FileID, unsigned> beginInfo = SM.getDecomposedLoc(Range.getBegin());
+  std::pair<FileID, unsigned> endInfo = SM.getDecomposedLoc(Range.getEnd());
----------------
naming NIT: use `BeginInfo`


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:625
+  RewriteRule R = makeRule(integerLiteral(equals(0)).bind(zero),
+                           change(node(zero), text("0")));
+  testRule(R, Input, Expected);
----------------
could we change to something other than `0` to make sure it's not the macro being expanded?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518