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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as done.
ymandel added a comment.

In D64518#1585917 <https://reviews.llvm.org/D64518#1585917>, @ilya-biryukov wrote:

> 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?


That's a really good question.  The code explicitly chooses to treat these failures like "this didn't match" rather than "this matched and now there's an error".  That reflects the split that some users will want to know while others will want the system to always skip such matches, just like it skips non-matching expressions.

This seems like a good candidate for configuration -- the user could then choose which mode to run in.  But, I'm also open to just reporting these conditions as errors.  It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".



================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+  const SourceManager &SM = *Result.SourceManager;
----------------
ilya-biryukov wrote:
> 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);
> ```
Sure. What do you think of exposing this function in clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518