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.

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

Add a missing error-return case. #41181

Closed
wants to merge 1 commit into from
Closed

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Apr 10, 2017

Fixes #41161.
It was a little mistake in #40815.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@@ -4763,6 +4763,8 @@ impl<'a> Parser<'a> {
err.span_label(sp, &"missing `fn`");
}
return Err(err);
} else if let Err(bang_err) = bang_err {
return Err(bang_err);
Copy link
Member

Choose a reason for hiding this comment

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

I feel that if this code used explicit match, mistake wouldn't have been made. Something like

 match (err, bang_err) {
    (Err(_), Err(_)) => ...
    (Err(_), _) => ...
    ...
}

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@pnkfelix
Copy link
Member

While I agree @nagisa has a good suggestion, I also think this change is simple enough and the ICE is problematic enough that I'd rather just land this as is.

@nagisa
Copy link
Member

nagisa commented Apr 14, 2017

Yeah, that’s why I didn’t do a “red cross, can’t land this”-sort of review and a commented instead.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 14, 2017

📌 Commit 1e19647 has been approved by pnkfelix

@pnkfelix
Copy link
Member

@bors rollup

@goffrie
Copy link
Contributor Author

goffrie commented Apr 14, 2017

Wait, I just made the change :p

@nagisa
Copy link
Member

nagisa commented Apr 14, 2017

@bors r-

@bors
Copy link
Contributor

bors commented Apr 14, 2017

🔑 Insufficient privileges

@pnkfelix
Copy link
Member

@bors r-

@arielb1
Copy link
Contributor

arielb1 commented Apr 16, 2017

There's #41282 in queue which also fixes this issue.

@goffrie goffrie closed this Apr 18, 2017
@goffrie goffrie deleted the issue-41161 branch April 18, 2017 00:57
@bors
Copy link
Contributor

bors commented Apr 18, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants