dblaikie added a comment. In D109345#2990577 <https://reviews.llvm.org/D109345#2990577>, @dexonsmith wrote:
> 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... Fair - probably my own hedging there. >>> 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. Oooh, right. Good point - thanks for walking me through it! > 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. Yeah, think I'm with you. Given the amount of churn this means, though - reckon it's worth it? Reckon it needs more llvm-dev thread/buy-in/etc? Any other bike-shedding for MemoryBufferCompat? (MemoryBufferDeprecated? but I don't really mind) 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
