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

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

dexonsmith added a comment.

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

> (were there other regressions I mentioned/should think about?)

I don't have specific concerns; I was just reading between the lines of your description...

>> 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?

Benefits I was seeing of splitting (1-3) up are:

- makes (2) easy for downstreams to integrate separately from (1) and (3) (see below for why (2) is interesting)
- prevents any reverts of (3) on main from resulting in churn in downstream efforts to migrate in response to (1-2)
- enables (3) to NOT be monolithic -- still debatable how useful it is, but if split up then individual pieces can run through buildbots separately (and be reverted separately)

>> 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?

(2) changes all downstream clients of MemoryBuffer APIs from `std::error_code` to `Error`

- Mostly will cause build failures
- Also runtime regressions, due to `auto`, etc.

The fix is to do a search/replace of `MemoryBuffer::` to `MemoryBufferCompat::` on only the downstream code.

- Splitting from (1) means you can sequence this change between (1) and (2) — code always builds.
- Splitting from (3) means you can do a simple search/replace. If (2) is packed up with (3) it seems a lot more awkward, since you have to avoid accidentally undoing (3) during the search/replace. Then if somehow (3) gets reverted you need to start over when it relands.

> 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