Fallible allocation by snf · Pull Request #48648 · rust-lang/rust

snf · 2018-03-01T23:30:44Z

Implementing RFC#2116 Fallible Allocation .
Work in progress. Initially adding @gankro's try_reserve for Vec.

rust-highfive · 2018-03-01T23:30:49Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

snf · 2018-03-03T00:27:47Z

I'm thinking if this should be behind a feature gate or not given the RFC was already approved. @Kimundi any idea?

Kimundi

Looks good!


Got confused by feature gates for new language features there. All new APIs should land as unstable, which automatically gives them the feature gate as written in the unstable attribute. So no extra work is needed, but marking it as a stable directly would be wrong. :)

Kimundi · 2018-03-03T10:15:58Z

You turn a unwrap() into a return with CollectionAllocErr::CapacityOverflow here, which I think technically changes the behaviour (or at least the panic message). Is this just a bugfix because this is also technically a capacity overflow?

In my understanding, if bucket allocation size cannot be calculated, it's a sign of capacity overflow. I don't think there is a regression risk here because the chances of hitting this case are probably very small. There are 2 things that will change here (1) non try_xxxx methods will have a different panic message (empty vs "capacity overflow") (2) the behaviour might change when oom=panic/abort is introduced.

Yeah, makes sense.

Kimundi · 2018-03-03T13:33:45Z

This needs to be the error message that the compiler actually outputs. use of unstable library feature "try_reserve" [...] or something like it.

Thanks, solved!

Kimundi · 2018-03-03T13:36:18Z

This should also reflect the error gotten from a unstable library feature.

Thanks, solved!

snf · 2018-03-03T22:50:53Z

I think this is all we want to do in this PR, I will create a separate one for oom=panic/abort :)

Kimundi · 2018-03-04T11:23:15Z

Seems there are still some import issues in the tests.

snf · 2018-03-07T18:37:56Z

r? @Kimundi . Thanks and sorry for all the messed up commits!, I will squash them after your review.

Kimundi · 2018-03-07T23:27:56Z

Spurious whitespace

Kimundi · 2018-03-08T01:42:41Z

Weird that that failed the tests again... But yeah, feel free to squash :)

snf · 2018-03-08T14:02:07Z

Ok, I'm squashing the changes into one

snf · 2018-03-08T16:22:14Z

Done :). r? @Kimundi

Kimundi · 2018-03-08T20:12:36Z

@bors r+

bors · 2018-03-08T20:12:37Z

📌 Commit 69c67d6 has been approved by Kimundi

bors · 2018-03-09T14:48:31Z

⌛ Testing commit 69c67d637065063979405eb6e9a6d2db11159b8a with merge e1bda06a39850023925b8d72aabdc7a56ae13145...

bors · 2018-03-09T14:58:22Z

💔 Test failed - status-travis

kennytm · 2018-03-09T15:09:48Z

Cannot run x.py tidy on i686 because it segfaults 🤔

[00:04:58] command did not execute successfully: "/checkout/obj/build/i686-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/i686-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
[00:04:58] expected success, got: signal: 11

kennytm · 2018-03-14T06:46:48Z

@bors retry

3 hour timeout

bors · 2018-03-14T07:24:20Z

⌛ Testing commit d4e400407a1c231fcf7f90b020b0e73411a5baec with merge 6319df5ff2bf0c4ef9fb8f95b5c2eae3f237cd90...

bors · 2018-03-14T08:20:19Z

💔 Test failed - status-travis

kennytm · 2018-03-14T08:23:45Z

Recent UI test changes again has broken UI tests 😠. Please rebase and update ui/feature-gate-try_reserve.stderr.

[00:51:10] failures:
[00:51:10] [00:51:10] ---- [ui] ui/feature-gate-try_reserve.rs stdout ----
[00:51:10] diff of stderr:
[00:51:10] [00:51:10] 8 [00:51:10] 9	error: aborting due to previous error
[00:51:10] 10 [00:51:10] -	If you want more information on this error, try using "rustc --explain E0658"
[00:51:10] +	For more information about this error, try `rustc --explain E0658`.
[00:51:10] 12

…d HashMap

snf · 2018-03-14T11:54:55Z

r? @Kimundi @kennytm . Confirmed the feature-gate error is now ok in my local build.

kennytm · 2018-03-14T13:19:19Z

@bors r=Kimundi

bors · 2018-03-14T13:19:20Z

📌 Commit 06057d9 has been approved by Kimundi

bors · 2018-03-15T08:19:07Z

⌛ Testing commit 06057d9 with merge a4af6f0...

Fallible allocation Implementing RFC#2116 [Fallible Allocation](#48043) . Work in progress. Initially adding @gankro's try_reserve for Vec.

bors · 2018-03-15T10:57:56Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Kimundi
Pushing a4af6f0 to master...

staktrace · 2018-03-16T21:32:24Z

@snf @Kimundi Is it expected that this PR causes build breakage for some crates? For example mp4parse-rust seems to have stopped building in nightly rust, see https://bugzilla.mozilla.org/show_bug.cgi?id=1446538

snf · 2018-03-16T23:25:12Z

@staktrace it's because of the conflicting names. I see that Mike commented that #48552 will fix it (and future breakages).
I'll check how mp4parse-rust is using try_reserve and probably submit a patch to use nightly's try_reserve if that's ok for you.

staktrace · 2018-03-17T14:36:07Z

@snf I don't have any objections but I'm also not the decision maker on this, I just happened to run into the build failure first. Maybe coordinate with the mp4parse-rust owners?

rust-highfive assigned Kimundi Mar 1, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2018

Kimundi approved these changes Mar 3, 2018
View reviewed changes

Kimundi reviewed Mar 3, 2018
View reviewed changes

Kimundi approved these changes Mar 7, 2018
View reviewed changes

Comment threadsrc/libsyntax/feature_gate.rs Outdated
KimundiMar 7, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spurious whitespace

snf force-pushed the fallible_allocation branch from 10a58d5 to 69c67d6Compare March 8, 2018 14:39

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 9, 2018

snf force-pushed the fallible_allocation branch from a86536f to f15bd05Compare March 10, 2018 21:42

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 14, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2018

snf added 3 commits March 14, 2018 03:48

implementing fallible allocation API (try_reserve) for Vec, String an…
92bfcd2
…d HashMap

try_reserve: disabling tests for asmjs, blocked byrust-lang#48968
b08b5ae

try_reserve: updating message for feature-gate-try_reserve.stderr
06057d9

snf force-pushed the fallible_allocation branch from 4455341 to 06057d9Compare March 14, 2018 10:49

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2018

bors added a commit that referenced this pull request Mar 15, 2018
Auto merge of#48648- snf:fallible_allocation, r=Kimundi
a4af6f0
Fallible allocation Implementing RFC#2116 [Fallible Allocation](#48043) . Work in progress. Initially adding @gankro's try_reserve for Vec.

bors mentioned this pull request Mar 15, 2018
Stabilize inclusive range (..=) #47813
Merged

bors merged commit 06057d9 into rust-lang:masterMar 15, 2018

aturon mentioned this pull request Mar 15, 2018
Tracking issue for try_reserve: RFC 2116 fallible collection allocation #48043
Open
5 tasks

Uh oh!

Uh oh!

Conversation

snf commented Mar 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 1, 2018

Uh oh!

snf commented Mar 3, 2018

Uh oh!

Kimundi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KimundiMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

snfMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

KimundiMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

KimundiMar 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snfMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

KimundiMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

snfMar 3, 2018

Choose a reason for hiding this comment

Uh oh!

snf commented Mar 3, 2018

Uh oh!

Kimundi commented Mar 4, 2018

Uh oh!

snf commented Mar 7, 2018

Uh oh!

KimundiMar 7, 2018

Choose a reason for hiding this comment

Uh oh!

Kimundi commented Mar 8, 2018

Uh oh!

snf commented Mar 8, 2018

Uh oh!

snf commented Mar 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kimundi commented Mar 8, 2018

Uh oh!

bors commented Mar 8, 2018

Uh oh!

bors commented Mar 9, 2018

Uh oh!

bors commented Mar 9, 2018

Uh oh!

kennytm commented Mar 9, 2018

Uh oh!

kennytm commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

kennytm commented Mar 14, 2018

Uh oh!

snf commented Mar 14, 2018

Uh oh!

kennytm commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

bors commented Mar 15, 2018

Uh oh!

bors commented Mar 15, 2018

Uh oh!

snf commented Mar 1, 2018•
edited
Loading

Kimundi left a comment •
edited
Loading

KimundiMar 3, 2018•
edited
Loading

snf commented Mar 8, 2018•
edited
Loading