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