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

[WIP] Require specific feature names in #[allow_internal_unstable]. #54714

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Oct 1, 2018

I opened this with just a hacky commit to test the perf impact of one implementation strategy.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2018
@eddyb
Copy link
Member Author

eddyb commented Oct 1, 2018

@bors try

@bors
Copy link
Contributor

bors commented Oct 1, 2018

⌛ Trying commit af5cce1 with merge 389686a...

bors added a commit that referenced this pull request Oct 1, 2018
[WIP] Require specific feature names in `#[allow_internal_unstable]`.

I opened this with just a hacky commit to test the perf impact of one implementation strategy.
@bors
Copy link
Contributor

bors commented Oct 1, 2018

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member Author

eddyb commented Oct 1, 2018

@rust-timer build 389686a

@rust-timer
Copy link
Collaborator

Success: Queued 389686a with parent f55129d, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Oct 2, 2018

We either don't have expansion stress tests... or I'm good to go.
cc @nikomatsakis @petrochenkov

@pnkfelix
Copy link
Member

pnkfelix commented Oct 5, 2018

@eddyb what is the next step here? Can you link this back to the relevant bug that sparked this investigation?

@petrochenkov
Copy link
Contributor

Can you link this back to the relevant bug that sparked this investigation?

+1, I'm mildly skeptical about this, why isn't a boolean flag enough?

@TimNN TimNN 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 Oct 16, 2018
@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2018

Ping from triage @eddyb: It looks like some clarifications have been requested for this PR.

@TimNN
Copy link
Contributor

TimNN commented Oct 23, 2018

Ping from triage @eddyb: What are your plans for this PR?

@TimNN
Copy link
Contributor

TimNN commented Oct 30, 2018

Ping from triage, @eddyb! This PR hasn't seen any updates in a while, so I'm closing it for now, per our guidelines. Thanks for your contributions and please feel free to re-open in the future.

@TimNN TimNN closed this Oct 30, 2018
@TimNN TimNN added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2018
@eddyb
Copy link
Member Author

eddyb commented Nov 3, 2018

The investigation was actually flawed, we'd need to also support library features, and those aren't a finite set, so they'd have to use Vec, BTreeSet or something else.

The problem I was considering solving, if at all possible efficiently, is that currently #[allow_internal_unstable] allows all features, which came up in #52011:

const fn feature-gating has to ignore allow_internal_unstable, otherwise panic!() will be usable in constant contexts on stable (which is supposed to be gated).
panic!() needs allow_internal_unstable but not for using the panic entry-point as a const fn.

In general, we might be accidentally allowing too much, and I wanted to limit that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants