Uh oh!
There was an error while loading. Please reload this page.
Swap the 2 variants of Option<T> - #49499
Conversation
eddyb
commented
Mar 30, 2018
cc @alexcrichton Can we do this without anyone observing it? |
There was a problem hiding this comment.
All methods below essentially have the same code, maybe dedup them with a macro?
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.
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.
petrochenkov
commented
Mar 30, 2018
Is |
nox
commented
Mar 30, 2018
@petrochenkov |
TimNN
commented
Mar 30, 2018
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 |
nox
commented
Mar 30, 2018
The failing test on Travis was due to rustc finding a different witness about the non-exhaustiveness, because both |
nox
commented
Mar 30, 2018
One question is, should we swap this one or |
TimNN
commented
Mar 30, 2018
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.
| { | ||
| #[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 forResult<T, ()>.
glandium
commented
Mar 31, 2018
This feels like a terrible idea. This moves statics initialized to None from |
e.g. |
scottmcm
commented
Mar 31, 2018
@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. |
oli-obk
commented
Mar 31, 2018
I just want to note that |
I kinda feel that relying on variant order is a bit fragile -- how do folks feel about a |
scottmcm
commented
Apr 1, 2018
I'm not sure I understand what the attribute would do, @Manishearth. Would it be like rust-lang/rfcs#2363? (Personally, I wish |
nox
commented
Apr 1, 2018
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. |
glandium
commented
Apr 1, 2018
How would you keep things like |
eddyb
commented
Apr 1, 2018
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. |
Manishearth
commented
Apr 1, 2018
@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. |
nox
commented
Apr 1, 2018
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 |
Manishearth
commented
Apr 1, 2018
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. |
Manishearth
commented
Apr 1, 2018
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 ( ._.). |
emilyalbini
commented
Apr 9, 2018
Ping from triage! What's the status on this PR? |
nox
commented
Apr 9, 2018
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.