dexonsmith added a comment.
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.
IMO, it'd be valuable to split out the consequential functional changes:
- Improvements/changes you made to FileError could be committed ahead of time
- Other improvements you discussed to avoid regressions in (e.g.)
llvm-symbolizer (seems potentially important?)
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. 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.)
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.
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.
WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109345/new/
https://reviews.llvm.org/D109345
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
