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

[David Blaikie via Phabricator via llvm-commits]

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