[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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