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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
