Swap the 2 variants of Option<T> #49499
Conversation
|
cc @alexcrichton Can we do this without anyone observing it? |
src/libcore/option.rs
Outdated
| @@ -981,6 +981,102 @@ impl<T> From<T> for Option<T> { | |||
| } | |||
| } | |||
|
|
|||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
There was a problem hiding this comment.
All methods below essentially have the same code, maybe dedup them with a macro?
| /// Some value `T` | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| Some(#[stable(feature = "rust1", since = "1.0.0")] T), | ||
| /// No value |
There was a problem hiding this comment.
Document (in a comment) the reason for this order and the manual impls below
There was a problem hiding this comment.
Should it a doc comment, or just a comment in the source?
There was a problem hiding this comment.
I added some comments and made them reference each other.
Manishearth
added
A-codegen
src/libcore/option.rs
Outdated
|
|
||
| // Note that this is not a lang item per se, but it has a hidden dependency on | ||
| // `Iterator`, which is one. The compiler assumes that the `next` method of | ||
| // `Iterator` is an enumeration with one type parameter and two variants, | ||
| // which basically means it must be `Option`. | ||
|
|
||
| /// The `Option` type. See [the module level documentation](index.html) for more. | ||
| #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)] | ||
| #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] |
There was a problem hiding this comment.
Hash might still observe the order?
There was a problem hiding this comment.
Hash offers no stability guarantees.
|
Is |
|
@petrochenkov |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
|
The failing test on Travis was due to rustc finding a different witness about the non-exhaustiveness, because both |
|
One question is, should we swap this one or |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
That allows us to have a better path from Result<T, E> to Option<T> in the general case, without niche-filling optimisations.
shepmaster
added
the
S-waiting-on-review
| { | ||
| #[inline] | ||
| fn cmp(&self, other: &Self) -> cmp::Ordering { | ||
| let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize; |
There was a problem hiding this comment.
Couldn't this use is_none() or is_some()?
There was a problem hiding this comment.
Yeah I guess I could use is_none, I'll check that it still produces the same code as for
Result<T, ()>.
|
This feels like a terrible idea. This moves statics initialized to None from |
|
e.g. |
|
@glandium Maybe that's a good way to answer #49499 (comment), since I assume |
|
@glandium What if I reversed the Result type then, would you feel it is a better idea?
Edit: didn't see Scott's comment at first, sorry for the dupe.
|
|
I just want to note that |
|
I kinda feel that relying on variant order is a bit fragile -- how do folks feel about a |
|
I'm not sure I understand what the attribute would do, @Manishearth. Would it be like rust-lang/rfcs#2363? (Personally, I wish |
This is what rust-lang/rfcs#2363 is for as Scott said, but I wanted to add that I don't want to block a codegen improvement on landing that RFC. We can very well rewrite the definition of both the Note that you would also need the second hook of 56f2875, so that enums with explicit discriminants don't get opted out of niche-filling optimisations. |
|
How would you keep things like |
|
Note that none of the null / niche / invalid value optimizations care about variant order and they have never cared for as far back as I can remember, years before 1.0. |
|
@scottmcm Yes, it is rust-lang/rfcs#2363 exactly, though as a lightweight attribute instead of a syntax change. @nox As long as it's unstable and only used internally it's not blocked on the RFC. This is also why I was suggesting an attribute -- it's easier to implement, and easier to remove. |
It's not easier to implement a new attribute than it is to swap 2 variants in an enum, especially if that attribute is a technical debt to be removed when my RFC lands. That being said I'm tempted to just close this one and reopen one for |
I'm not saying it is, it's easier to implement than the RFC itself.
If your RFC lands you would probably be able to reuse the implementation, and replace the attribute with a hard syntax change. |
|
To be clear: I don't want to block this on that; but I do feel that in the long term we want something less fragile here, and I feel that implementing that RFC as an attribute (or implementing it entirely, either way), but keeping it behind a feature flag, is the best way to go forward. Either in this PR or as a followup issue. There is more precedent for implementing lang features as attributes for stdlib optimizations. |
|
I've thought a bit more about it, and implementing the niche-filling optimisation on enums with explicit discriminants is more complex than I thought it would be, so I think we should still try to just reverse the 2 variants of one of the two enums /me goes back to filing WebGL issues, please don't nerdsnipe him ( ._.). |
|
Ping from triage! What's the status on this PR? |
|
I'll reopen it later as a |
That allows us to have a better path from
Result<T, E>toOption<T>in the general case, without niche-filling optimisations.@rust-lang/wg-codegen
It should be noted that if you also have #49479 and #49420 (not sure this one matters but it was in my branch so full disclosure I guess), the methods for
Result<T, UnitStuff>andOption<T>end up being shared forTvalues that don't trigger the niche-filling optimisation.