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

remove associated_consts feature gate #42809

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

seanmonstar
Copy link
Contributor

Currently struggling to run tests locally (something about jemalloc target missing).

cc #29646

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@seanmonstar seanmonstar force-pushed the stable-associated-consts branch 3 times, most recently from 0d9a786 to 4f9a4c0 Compare June 22, 2017 00:35
@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2017
@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

Deleting the associated_consts page of the unstable book altogther? Shouldn't the stable book be updated somehow to account for them?

@seanmonstar
Copy link
Contributor Author

Sure, though as to where and how, I have no idea. And it seems all the possible places to move that file are actually submodules...

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

I'd suggest perhaps just leaving the page in the unstable book for now so that we don't forget that documentation exists for it, after all, it is still unstable until the 1.20 release. Docs are low-risk to backport to beta, if it comes to that.

@bstrie
Copy link
Contributor

bstrie commented Jun 23, 2017

I don't think it's worth holding up this PR for this next comment, but there's a postponed RFC from 2015 that was gated on associated consts that could be worth discussing now ( rust-lang/rfcs#1168 ), but for the sake of backcompat it wants to reserve two legal identifiers for associated consts for future use. Again, not worth holding up this PR for, but something that ought to be considered before this makes it into beta.

@seanmonstar
Copy link
Contributor Author

make tidy fails if I leave the page in the unstable-book.

@bors
Copy link
Contributor

bors commented Jun 23, 2017

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

@seanmonstar seanmonstar force-pushed the stable-associated-consts branch from 4f9a4c0 to 79cab7a Compare June 25, 2017 18:01
@arielb1
Copy link
Contributor

arielb1 commented Jun 27, 2017

Travis failure is spurious. r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

@seanmonstar sorry for the delay!

Before stabilizing, we are supposed to have doc PRs pending, at least. There are some details about how to do it in the forge docs on stabilization. I think the basic idea would be to move the existing material from the "unstable book" into the reference -- sparse as it is, it's better than nothing.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Other than the lack of a documentation PR, this seems good!

@bors
Copy link
Contributor

bors commented Jun 30, 2017

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

@seanmonstar
Copy link
Contributor Author

Rebased again, and opened a PR against the reference to pull in the docs from the unstable book: rust-lang/reference#75

@bors
Copy link
Contributor

bors commented Jul 6, 2017

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

@est31
Copy link
Member

est31 commented Jul 6, 2017

@seanmonstar just that you are aware for the case you want this to get into 1.20 : the beta branches in a week.

@seanmonstar seanmonstar force-pushed the stable-associated-consts branch from c23810d to 9386be0 Compare July 6, 2017 16:02
@seanmonstar
Copy link
Contributor Author

@est31 oh, I know! After yet another rebase, I'll ping someone with merge permissions when travis is green.

@seanmonstar seanmonstar force-pushed the stable-associated-consts branch from 9386be0 to 42eccb5 Compare July 6, 2017 17:34
@seanmonstar seanmonstar force-pushed the stable-associated-consts branch from 42eccb5 to 74b2d69 Compare July 6, 2017 18:52
@seanmonstar
Copy link
Contributor Author

@nikomatsakis ok, so it's green, and the PR to the reference is approved waiting on this merge. We good to go?

@frewsxcv frewsxcv added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 7, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 7, 2017

📌 Commit 74b2d69 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 7, 2017

⌛ Testing commit 74b2d69 with merge 13157c4...

bors added a commit that referenced this pull request Jul 7, 2017
…tsakis

remove associated_consts feature gate

Currently struggling to run tests locally (something about jemalloc target missing).

cc #29646
@bors
Copy link
Contributor

bors commented Jul 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 13157c4 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants