Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards dynamic const-qualify #27
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
Towards dynamic const-qualify #27
Changes from 9 commits
a18aafc
a9c7047
50a1829
cbd7e7e
53d0200
7f8c81c
87c3c87
95acdf2
a29f147
1d2b7b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung I think my confusion really occurs because of this paragraph. It seems to apply that both
const
s should compile in the following example, which is not true.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. That's because of the
Drop
check, which still applies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also fails on types which are notMissing aDrop
. I think @oli-obk's comment above was incorrect?const
. Ignore me plz.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That example works if you make
new
aconst fn
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "enclosing scope" rule doesn't make sense to me. I don't know how it should extend to arbitrary cases. Is it the braces that cause the problem or the assignment to
x
? It also might be good to mention#[rustc_promotable]
here, since without it calls toconst fn
s can't get promoted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I am paraphrasing @eddyb here.
@eddyb are these rules explained anywhere?
Good point, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I guess I should edit this in the light of our unresolved terminology.
Is the entire "Promotion is not involved" incorrect, or is there something there that makes sense? I was trying to explain why
const EMPTY_BYTES: &Vec<u8> = &Vec::new();
works even though this stuff does not get promoted. @ecstatic-morse since my explanation confused you, could you try to explain this in your own words?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my latest comment for what is actually confusing me here. The "enclosing scope" rule makes sense intuitively, but the details are still not clear to me. That's my problem though, not this PR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done some edits. @ecstatic-morse does this resolve your confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enclosing scope rule is the same one that makes these two different:
Except instead of
let
it'sconst
/static
and instead of "enclosing block" it's'static
.I don't remember the exact name of these semantics, maybe "rvalue/temporary lifetime/scope"? @nikomatsakis would be more helpful here, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call these "temporary lifetimes" -- I've been meaning to writeup some text about them for the rust reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call these "temporary lifetimes" -- I've been meaning to writeup some text about them for the rust reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be nice personally; less times to invent an ephemeral constant just to please the check and you also get to use exported statics from other crates.
Any future compat hazards with some new feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discovered a problem... If we want to have a scheme like rust-lang/rfcs#2492 but centered around
static
s instead, allowingstatic
s in const contexts will give us all the problems re. separate compilation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Centril sorry, I do not follow... why is it harder for separate compilation to support
const
referring tostatic
thanconst
referring toconst
(and the latter we already support)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it would be a
static
for which the value isn't known yet; that is something likepub extern static Foo: u8;
<-- no value. That would be similar to rust-lang/rfcs#2492 in that it is matched up with something in the crate graph. If you can then refer toFoo
in types then that will create all the same problems as withextern existential type
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as whatever instantiates the
static
checks them for validity, we are sill good. The important part is the invariant that all statics have been checked. The order doesn't matter.