Skip to content

generalize the Option<NonNullablePtr> to libs #8678

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

Closed
wants to merge 1 commit into from
Closed

generalize the Option<NonNullablePtr> to libs #8678

wants to merge 1 commit into from

Conversation

thestinger
Copy link
Contributor

An Option<Rc<T>> is now 1-word, like the built-in non-nullable pointers.

@alexcrichton
Copy link
Member

This seems like a pretty cool feature, but it's also a little worrying to me. Right now you can declare unsafe_no_drop_flag and unsafe_non_zero_word and the compiler performs 0 checks to make sure you do anything sanely. I would expect unsafe_non_zero_word to at least verify that the struct's size is only 1 word.

It does make sense to flag them with unsafe, but it's not immediately clear why these attributes have unsafe in front of them. With unsafe blocks the compiler will tell you the specific reason that you want them, but with these attributes the compiler will just silently assume that you're obeying whatever contract is necessary.

I'm not really sure what the best answer to that is. Arguably the only way to actually learn of the attributes would be to read about them in some documentation, so the best solution could be to just document them well.

@thestinger
Copy link
Contributor Author

I think you could also use this on a multiple-word struct as long as the first word was always zero. Although, it's not explicitly supported yet because I don't know a use case.

@huonw
Copy link
Member

huonw commented Aug 22, 2013

It seems like it would be relatively easy to pass the crate ctxt rather than the ty ctxt to give an error for size != 1 word.

@thestinger
Copy link
Contributor Author

It would be equally unsafe with a check like that though.

@pcwalton
Copy link
Contributor

I vote making sure it's 1 word for now to be conservative. BTW, @nikomatsakis and I would like to remove the drop flag entirely, rendering this moot.

@nikomatsakis
Copy link
Contributor

Indeed. I am in fact actively working on this, though there is an annoyingly long set of blocker bugs in between here and there.

@nikomatsakis
Copy link
Contributor

But does that truly render the issue moot? It would still affect how much space Option<RC<T>> takes.

@catamorphism
Copy link
Contributor

I'm hearing that this patch will be subsumed by @nikomatsakis 's work-in-progress, so I'm closing the PR. Re-open if necessary.

@thestinger
Copy link
Contributor Author

@catamorphism: I don't think it will - this is about making Option<Rc<int>> be 1 word instead of 2, like we do for &, @ and ~.

It does work but it could provide error messages on types that are not 1 word, instead of unspecified behaviour.

@thestinger thestinger reopened this Sep 12, 2013
@catamorphism
Copy link
Contributor

Closing again due to lack of activity. Please do reopen if you're able to make progress, @thestinger !

@thestinger
Copy link
Contributor Author

I will eventually :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants