Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[breaking-batch] Move more uses of panictry! out of libsyntax #31631

Merged
merged 2 commits into from
Mar 9, 2016
Merged

[breaking-batch] Move more uses of panictry! out of libsyntax #31631

merged 2 commits into from
Mar 9, 2016

Conversation

jonas-schievink
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@jonas-schievink
Copy link
Contributor Author

...I messed that up. Fixing!

@alexcrichton
Copy link
Member

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned sfackler Feb 13, 2016
@jonas-schievink
Copy link
Contributor Author

This should work. I'm not too happy about panictry! infecting even more crates, but since the Err case propagates a DiagnosticBuilder which causes side effects (or must cause side effects, otherwise it panics), I'm not sure how to handle the PResult otherwise.

@bors
Copy link
Contributor

bors commented Feb 15, 2016

☔ The latest upstream changes (presumably #31530) made this pull request unmergeable. Please resolve the merge conflicts.

@jonas-schievink
Move more uses of panictry! out of libsyntax
3a87278 
[breaking-change] for syntax extensions
@jonas-schievink
Copy link
Contributor Author

Rebased

nrc
nrc reviewed Feb 19, 2016
View reviewed changes
src/librustc_driver/driver.rs
@@ -86,7 +84,7 @@ pub fn compile_input(sess: &Session,
// possible to keep the peak memory usage low
let (outputs, trans) = {
let (outputs, expanded_crate, id) = {
let krate = phase_1_parse_input(sess, cfg, input);
let krate = panictry!(phase_1_parse_input(sess, cfg, input));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't panic here, we should return an error instead

@nrc
Copy link
Member

nrc commented Feb 19, 2016

r+ with the above changes (sorry the review took so long).

@nrc
Copy link
Member

nrc commented Mar 9, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 9, 2016

📌 Commit 11e0ba4 has been approved by nrc

@bors
Copy link
Contributor

bors commented Mar 9, 2016

⌛ Testing commit 11e0ba4 with merge cbbd3d9...

bors added a commit that referenced this pull request Mar 9, 2016
@bors
Auto merge of #31631 - jonas-schievink:agoraphobia, r=nrc
cbbd3d9 
[breaking-batch] Move more uses of `panictry!` out of libsyntax
@bors
Copy link
Contributor

bors commented Mar 9, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 11e0ba4 into rust-lang:master Mar 9, 2016
@jonas-schievink jonas-schievink deleted the agoraphobia branch March 9, 2016 15:15
@jonas-schievink
Copy link
Contributor Author

Ah, dang it, I totally forgot about #31645, now this'll break stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nrc nrc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jonas-schievink @rust-highfive @alexcrichton @bors @nrc @sfackler