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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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

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


Ah, great, we're on the same page then. LGTM!

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

Yes, exactly.

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

I think it depends on the use-case, e.g. if we try to produce a clang-tidy fix for some warning and can't produce a fix because `makeValidRange` failed, then not showing the fix (i.e. failing silently) seems fine.
OTOH, if we're building a refactoring tool that should find an replace all occurrences of a matcher and apply the transformation, failing silently is probably not a good option, we should possibly list the locations where the transformation failed (so that users can do manual changes to complete the refactoring).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64518