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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 17 11:10:30 PDT 2019

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list