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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

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

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


I think we might be talking about different things here. I meant that the *calling* function, `translateEdits`, returns `Expected`, so it would be easy to return an error when `makeValidRange` returns `None`.  I agree that `makeValidRange` (or whatever we choose to call it) should stick with `Optional` for simplicity (with the generic interpretation of `None` being "could not map from macro expansion to source code").

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

I assume you mean which cases `makeValidRange` should handle (successfully)? If so, that sounds good.  But, what do you think about how to handle failures of `makeValidRange` -- ignore them silently (which is what we're doing now) or treat them as errors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518