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

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

dexonsmith added a comment.

In D109345#2990612 <https://reviews.llvm.org/D109345#2990612>, @dblaikie wrote:

> Given the amount of churn this means, though - reckon it's worth it? Reckon it needs more llvm-dev thread/buy-in/etc?

I think the churn is worth since my intuition is that it has high value for downstreams/clients (in terms of mitigating risk/etc.). (For example, the Swift compiler also uses MemoryBuffer and uses `auto` quite heavily.)

Not sure if this needs more buy-in, but probably worth communicating the plan on llvm-dev. If nothing else, makes it easy for relevant commits to point to it on lists.llvm.org. Could even add a working `sed -e` or `perl` command for the renames?

> Any other bike-shedding for MemoryBufferCompat? (MemoryBufferDeprecated? but I don't really mind)



- MemoryBufferDeprecated SGTM (more descriptive than "compat"), MemoryBufferLegacy would work too
- MemoryBufferErrorCodeAPI is even more descriptive -- see related idea below
- could also rename all the (relevant) functions instead of the class... but since these are all `static` anyway renaming the class feels easier?

Thinking the names through gave me an idea for a refined staging:

1. Add MemoryBufferErrorAPI (wrapping APIs with errorOrToExpected) and MemoryBufferErrorCodeAPI (alias for MemoryBuffer) in the same commit.
2. Migrate in-tree callers to MemoryBufferErrorCodeAPI (via mass rename). (Could even move some to MemoryBufferErrorAPI?)
3. Update MemoryBuffer to use Error/Expected APIs, change MemoryBufferErrorAPI to an alias of it, and leave behind MemoryBufferErrorCodeAPI (wrapping APIs with expectedToErrorOr).
4. One or more commits:
  1. Migrate in-tree callers to MemoryBuffer.
  2. Delete MemoryBufferErrorAPI alias.
5. Delete MemoryBufferErrorCodeAPI wrappers.

The refinement is that the new (1) is better designed for cherry-picking to a stable branch:

- Isolated to MemoryBuffer (no changes to callers), making conflict resolution trivial.
- Downstreams / clients can migrate to MemoryBufferError without integrating the change to the default behaviour of MemoryBuffer.
- Particularly useful for clients that want to cherry-pick/merge changes between a branch that builds against ToT LLVM and one that builds against a "stable" version that predates the transition.

The new (3) and (5) are the same as the old (2) and (4) -- isolated changes that are easy to revert.

(But if you're not convinced, I think my previous idea for staging would still be a huge help.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109345/new/

https://reviews.llvm.org/D109345