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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
