dblaikie added a comment.
In D109345#2987565 <https://reviews.llvm.org/D109345#2987565>, @dexonsmith
wrote:
> This seems like the right direction to me! Especially like the
> look-through-the-ErrorInfo change for `FileError` -- I hit this before and
> found it annoying.
Thanks for taking a look!
> IMO, it'd be valuable to split out the consequential functional changes:
>
> - Improvements/changes you made to FileError could be committed ahead of time
Sure sure, can be committed and unit tested separately for sure.
> - Other improvements you discussed to avoid regressions in (e.g.)
> llvm-symbolizer (seems potentially important?)
I didn't think the yaml symbolizer output was that important - but turned out
not to be super hard to fix, so I've done that. (were there other regressions I
mentioned/should think about?)
> I agree the other changes are mostly mechanical and don't all need careful
> review-by-subcomponents.
>
> That said, it looks very painful for downstream clients of the LLVM C++ API
> since it's structured as an all-or-nothing change.
Yeah, certainly crossed my mind.
> Especially for managing cherry-picks to long-lived stable branches. It's
> painful because clients will have code like this:
>
> if (auto MaybeFile = MemoryBuffer::getFileOrSTDIN()) {
> // Do something with MaybeFile
> }
> // Else path doesn't care about the specific error code.
>
> that will suddenly start crashing at runtime. I even wonder if there like
> that introduced in-tree by your current all-in-one patch, since I think our
> filesystem-error paths are often missing test coverage. (It'd be difficult to
> do a thorough audit.)
Yeah, that came up in a handful of cases that used 'auto' (without using auto
it's a compile failure).
> I thought through a potential staging that could mitigate the pain:
>
> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all
> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct
> impact on downstreams.
Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but
yeah, it's probably worthwhile.
What's the benefit of having the extra step where everything's renamed twice?
(if it's going to be a monolithic commit - as mentioned in (3)) Compared to
doing the mass change while keeping the (1 & 2) pieces for backwards
compatibility if needed?
> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind
> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef
> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and
> cherry-pick once they've finished adapting in the example of (1).
> 3. Update all code to use the new APIs. Could be done all at once since it's
> mostly mechanical, as you said. Also an option to split up by component
> (e.g., maybe the llvm-symbolizer regression is okay, but it could be nice to
> get that reviewed separately / in isolation).
> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while they
> follow the example of on (3).
>
> (Given that (1) and (2) are easy to write, you already have `tree` state for
> (4), and (3) is easy to create from (4), then I //think// you could construct
> the above commit sequence without having to redo all the work... then if you
> wanted to split (3) up from there it'd be easy enough.)
>
> (2) seems like the commit mostly likely to cause functional regressions, and
> it'd be isolated and easy to revert/reapply (downstream and/or upstream)
> without much churn.
(3) would be more likely to cause regression? Presumably (2) is really
uninteresting because it adds a new API (re-adding MemoryBuffer, but unused
since everything's using MemoryBufferCompat) without any usage, yeah?
Plausible, though a fair bit more churn - I'd probably be up for it, though.
- Dave
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109345/new/
https://reviews.llvm.org/D109345
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
