[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#3008426 , @dblaikie wrote:

> Thanks for the suggestions/details, @dexonsmith  - I've posted to llvm-dev 
> here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and 
> will wait for some follow-up (or dead silence) before starting along probably 
> your latter suggestion.

SGTM, thanks David!


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for the suggestions/details, @dexonsmith  - I've posted to llvm-dev 
here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and 
will wait for some follow-up (or dead silence) before starting along probably 
your latter suggestion.


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

[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#2992274 , @dexonsmith 
wrote:

> 4. One or more commits:
>   1. Migrate in-tree callers to MemoryBuffer.
>   2. Delete MemoryBufferErrorAPI alias.
> 5. Delete MemoryBufferErrorCodeAPI wrappers.

(Potentially MemoryBufferErrorAPI and MemoryBufferErrorAPI could be kept across 
an LLVM release branch boundary to help LLVM clients that use those... not sure 
how useful that'd be?)


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

[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#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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D109345#2990577 , @dexonsmith 
wrote:

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

[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 , @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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

`ninja clang` on Windows works. `check-llvm-tools` has a few `REQUIRES: 
system-windows` tests and I am running them. Test invocation is bit slow.


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

[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.

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.

IMO, it'd be valuable to split out the consequential functional changes:

- Improvements/changes you made to FileError could be committed ahead of time
- Other improvements you discussed to avoid regressions in (e.g.) 
llvm-symbolizer (seems potentially important?)

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. 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.)

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

WDYT?


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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D109345#2986297 , @thopre wrote:

> Is there no way to split this patch further? It's going to be hard finding 
> someone who can review something so big. If there's no way to split it in 
> incremental changes, you could perhaps split per subsystem only for review 
> and refer to this diff for CI as well as when landing.

The long migration path would be to do this one function at. time (I did a 
whole cluster of functions in MemoryBuffer for consistency - this does reduce 
total code changes somewhat, since some of those APIs are used in similar 
contexts (eg: branches of a conditional operator - so having them differ means 
more revisions to those call sites)) and probably introducing separate names 
for the Expected versions of the functions, migrating call sites incrementally, 
then doing a mechanical rename at the end of all that.

I don't think it's probably worth that level of granularity - it's a fairly 
mechanical patch as it is.

Mostly I sent this out as an FYI and to get feedback on the general direction - 
whether folks thought it was worth doing at all, and if they feel strongly it 
should be done another way (like the incremental ones above) - but I don't 
think it needs a /huge/ amount of scrutiny, review by separate code owners, 
etc. I'd generally be comfortable committing this as other cross-project 
cleanup/refactoring like function renaming, etc.


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

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

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added a comment.

Is there no way to split this patch further? It's going to be hard finding 
someone who can review something so big. If there's no way to split it in 
incremental changes, you could perhaps split per subsystem only for review and 
refer to this diff for CI as well as when landing.


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