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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D64518#1588092 <https://reviews.llvm.org/D64518#1588092>, @ymandel wrote:

> 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".


WRT to returning `Expected` vs `Optional`. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose `Optional` let's stick to it, as practice shows switching from `Optional` to `Expected` correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with `Optional` here (meaning the client code would have to say something generic like `could not map from macro expansion to source code`). But up to you, not a strong preference.

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?



================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+  const SourceManager &SM = *Result.SourceManager;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518