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

stabilise ?, attributes on stmts, deprecate Reflect #36995

Merged
merged 4 commits into from
Oct 13, 2016
Merged

stabilise ?, attributes on stmts, deprecate Reflect #36995

merged 4 commits into from
Oct 13, 2016

Conversation

nrc
Copy link
Member

@nrc nrc commented Oct 6, 2016

@nrc nrc mentioned this pull request Oct 6, 2016
Open
Manishearth
Manishearth approved these changes Oct 6, 2016
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

lgtm, not sure if libs/lang team folks need to sign off. r=me otherwise

src/librustc/diagnostics.rs
found enum `core::result::Result`) [E0308]
```

`try!` returns a `Result<T, E>`, and so the function must. But `main()` has
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this text apply to ? as well?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe he removed it because its not accurate anymore: #36988

Copy link
Member Author

Choose a reason for hiding this comment

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

No, annoyingly you get an error message about the Carrier trait

petrochenkov
petrochenkov reviewed Oct 6, 2016
View reviewed changes
src/libcore/macros.rs
@@ -255,6 +255,9 @@ macro_rules! debug_assert_ne {
/// Helper macro for reducing boilerplate code for matching `Result` together
/// with converting downstream errors.
///
/// Prefer using `?` syntax to `try!`. `?` is built in to the language and is
/// more succinct than `try!`, it is the standard method for error propogation.
Copy link
Contributor

Choose a reason for hiding this comment

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

propogation -> propagation

Copy link
Member

Choose a reason for hiding this comment

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

? is built in to the language and is more succinct than try!, it is the standard method for error propogation.

These are two separate sentences and probably shouldn't be separated by a comma.

@frewsxcv
Copy link
Member

frewsxcv commented Oct 6, 2016

For the sake of cross-linking, here's the link to the tracking issue: #31436

@nrc
Copy link
Member Author

nrc commented Oct 6, 2016

Also cc #15701, #27749

@nrc nrc added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Oct 6, 2016
@bors
Copy link
Contributor

bors commented Oct 7, 2016

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

@nikomatsakis
Copy link
Contributor

This looks good to me, though I do think we ought to try and address #36988.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2016

📌 Commit adb4e2f has been approved by nikomatsakis

@bluss
Copy link
Member

bluss commented Oct 11, 2016

What's the type inference situation? It used to be that replacing try!(x) with x? would not infer as often.

Has this been cratered to look for such issues?

@nrc
Copy link
Member Author

nrc commented Oct 11, 2016

@bluss good point, I had mis-remembered this issue. We have indeed done a bunch of Crater runs, but to test whether ? is forwards compatible with the Carrier trait, which requires some backwards incompatibility between try! and ?. I don't think we can ever replace the implementation of try! in a backwards compatible way and that shouldn't be in this PR.

@bors: r-

@nrc
Stabilise ?
9bc6d26 
cc [`?` tracking issue](rust-lang#31436)
@nrc
Stabilise attributes on statements.
a94f593 
Note that attributes on expressions are still unstable and are behind the `stmt_expr_attributes` flag.

cc [Tracking issue](rust-lang#15701)
@nrc
Deprecate Reflect
14c62f9 
[tracking issue](rust-lang#27749)
@nrc
Review changes
79b5177
@nrc
Copy link
Member Author

nrc commented Oct 11, 2016

@bors: r=@nikomatsakis

@bors
Copy link
Contributor

bors commented Oct 11, 2016

📌 Commit 79b5177 has been approved by @nikomatsakis

@alexcrichton alexcrichton mentioned this pull request Oct 12, 2016
Merged
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 12, 2016
@alexcrichton
Rollup merge of rust-lang#36995 - nrc:stable, r=@nikomatsakis
9d70ff3 
stabilise ?, attributes on stmts, deprecate Reflect

r? @nikomatsakis
bors added a commit that referenced this pull request Oct 12, 2016
@bors
Auto merge of #37118 - alexcrichton:rollup, r=alexcrichton
d34318d 
Rollup of 17 pull requests

- Successful merges: #36762, #36831, #36973, #36991, #36995, #37023, #37049, #37050, #37056, #37064, #37066, #37067, #37084, #37089, #37091, #37092, #37110
- Failed merges:
@nikomatsakis
Copy link
Contributor

@nrc

I don't think we can ever replace the implementation of try! in a backwards compatible way and that shouldn't be in this PR.

argh, I missed that this change was even in the PR! good catch @bluss

@bors bors merged commit 79b5177 into rust-lang:master Oct 13, 2016
@nrc nrc mentioned this pull request Oct 18, 2016
Merged
This was referenced Oct 18, 2016
Merged
Open
@brson brson mentioned this pull request Oct 19, 2016
Merged
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 19, 2016
@brson brson mentioned this pull request Nov 7, 2016
Merged
dtolnay added a commit to dtolnay/erased-serde that referenced this pull request Dec 24, 2016
@dtolnay
Update reason for not using TypeId
101e0d7 
Reflect is no longer required as of rust-lang/rust#36995.
@pnkfelix pnkfelix mentioned this pull request Sep 22, 2018
Closed
@ehuss ehuss mentioned this pull request Mar 3, 2020
Closed
@chinoto chinoto mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@est31 est31 est31 left review comments

@Manishearth Manishearth Manishearth approved these changes

@msjyoo msjyoo msjyoo approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@nrc @frewsxcv @bors @nikomatsakis @bluss @Manishearth @msjyoo @petrochenkov @est31 @alexcrichton