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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 3 inline comments as done.
ymandel added inline comments.


================
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:
> ymandel wrote:
> > 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?
> Sounds reasonable. Was thinking of a better name, maybe something like `getRangeForEdit()`?
> Would also suggest to accept `SourceManager` and `LangOptions` instead of `MatchResult` to narrow down the requirements on the clients.
Went with passing the ASTContext rather than the MatchResult (in the new revision D64924)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518