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

Allow and implement recursive static variables. #26630

Merged
merged 3 commits into from
Jul 25, 2015

Conversation

eefriedman
Copy link
Contributor

_Edit: Fixed now._ I'm pretty sure the way I'm using LLVMReplaceAllUsesWith here is
unsafe... but before I figure out how to fix that, I'd like a
reality-check: is this actually useful?

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@eddyb
Copy link
Member

eddyb commented Jun 28, 2015

This would be really neat for &'static cycles, e.g. a safe doubly linked static list.

@eefriedman eefriedman changed the title [WIP] Allow and implement recursive static variables. Allow and implement recursive static variables. Jun 28, 2015
@eefriedman
Copy link
Contributor Author

Updated; should be safe now. Also added an extra commit to remove the restriction on types which can't be instantiated, to allow implementing a static doubly-linked list.

@alexcrichton
Copy link
Member

cc @rust-lang/lang, not sure if this is a large enough change to warrant an RFC, it'd probably be nice to have a "static specification" but that may be a bit of a pipe dream.

@nrc
Copy link
Member

nrc commented Jun 29, 2015

I don't think it needs an RFC and I think it is a good idea. I do think it probably deserves some eyes on it, so I created an announcement in our internals discourse: https://internals.rust-lang.org/t/heads-up-allowing-recursive-static-variables/2309

@ftxqxd
Copy link
Contributor

ftxqxd commented Jun 30, 2015

Are nonsensical cycles like static FOO: T = FOO; and static FOO: T = BAR; static BAR: T = FOO; still disallowed by this PR?

Also, perhaps the tests that are deleted in this PR should instead just be moved to run-pass.

@eefriedman
Copy link
Contributor Author

@P1start The initializer for a static item can't read the contents of any static item, so static FOO: T = FOO; is still rejected.

Not sure the tests are particularly useful, but I guess I could move them.

@eefriedman
Copy link
Contributor Author

Tests re-added.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 2, 2015

Surely this should at least be feature gated?

@pnkfelix
Copy link
Member

pnkfelix commented Jul 2, 2015

(that is, I can understand side-stepping the RFC process, though I'm not 100% sure I agree with doing so, even for a feature like this. But with or without an RFC, it still seems prudent to feature-gate new functionality in the language, just so that we can kick the tires on it in nightly without worry about whether it could inadvertantly leak out into the other distribution channels.)

@eefriedman
Copy link
Contributor Author

static_recursion feature gate added. I strongly doubt there will be any issues in practice, but better to be on the safe side, I guess.

@eddyb
Copy link
Member

eddyb commented Jul 3, 2015

AFAICT you had to revert the removal of is_instantiable. I guess it will go away once the feature gate is lifted.

@bors
Copy link
Contributor

bors commented Jul 7, 2015

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

@pnkfelix
Copy link
Member

pnkfelix commented Jul 8, 2015

@eefriedman sorry I have not reviewed this yet. I hope I will get to it this week but I am not sure if I will.

@eefriedman
Copy link
Contributor Author

@pnkfelix Ping.

@pnkfelix
Copy link
Member

@eefriedman sorry for the delay

@@ -131,6 +128,11 @@ pub struct LocalCrateContext<'tcx> {
/// Cache of closure wrappers for bare fn's.
closure_bare_wrapper_cache: RefCell<FnvHashMap<ValueRef, ValueRef>>,

/// List of globals for static variables which need to be RAUW'ed when
Copy link
Member

Choose a reason for hiding this comment

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

nit: as far as I can tell the acronym "RAUW" does not occur elsewhere in our code.

So, please expand this occurrence and then include the acronym afterwards in parentheses, effectively defining the acronym. Like so: "static variables which need to be passed through the LLVM Replace All Uses With (RAUW) functionality"

@pnkfelix
Copy link
Member

okay looks good as far as my knowledge of LLVM goes.

r=me after next rebase (and hopefully the noted nit is addressed).

There isn't any particularly good reason for this restriction, so just
get rid of it, and fix trans to handle this case.
The borrow checker doesn't allow constructing such a type at runtime
using safe code, but there isn't any reason to ban them in the type checker.

Included in this commit is an example of a neat static doubly-linked list.

Feature-gated under the static_recursion gate to be on the safe side, but
there are unlikely to be any reasons this shouldn't be turned on by
default.
@eefriedman
Copy link
Contributor Author

@pnkfelix Rebased.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2015

📌 Commit 0eea0f6 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Jul 25, 2015

⌛ Testing commit 0eea0f6 with merge e333e6a...

bors added a commit that referenced this pull request Jul 25, 2015
***Edit: Fixed now.*** I'm pretty sure the way I'm using LLVMReplaceAllUsesWith here is
unsafe... but before I figure out how to fix that, I'd like a
reality-check: is this actually useful?
@bors bors merged commit 0eea0f6 into rust-lang:master Jul 25, 2015
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants