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

Clean up collections::EnumSet #19679

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Dec 9, 2014

Make EnumSet portable by using a fixed-size unsigned integer, namely u32.
The previous choice of uint creates portability hazards as too high enum
variants are only detected at runtime.

Mark the from_u32 method of the trait CLike as unsafe, as it is supposed to
be implemented without checking, thus being unsafe.

Remove the memory-unsafe Decode implementation for EnumSet.

This is a [breaking-change]. Code using EnumSet needs to be updated to use
u32s.

Make `EnumSet` portable by using a fixed-size unsigned integer, namely `u32`.
The previous choice of `uint` creates portability hazards as too high enum
variants are only detected at runtime.

Mark the `from_u32` method of the trait `CLike` as unsafe, as it is supposed to
be implemented without checking, thus being unsafe.

Remove the memory-unsafe `Decode` implementation for `EnumSet`.

This is a [breaking-change]. Code using `EnumSet` needs to be updated to use
`u32`s.
@Gankra
Copy link
Contributor

Gankra commented Dec 10, 2014

I wonder if EnumSet should be generic over an arbitrary UnsignedInt implementor. That way you can have a u8, u32, u64, whatever.

CC @SimonSapin or @sfackler I have no idea what EnumSet is really "for" anymore.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 10, 2014

The only current use is in the compiler, at the position that is changed by this commit as well.

@Gankra
Copy link
Contributor

Gankra commented Dec 10, 2014

I believe Servo and others use it.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 10, 2014

This doesn't remove functionality for software that is supposed to run on both 32/64 bit systems. So this change should only affect 64-bit only software (but there's not a lot of that (?)).

@SimonSapin
Copy link
Contributor

EnumSet is not currently used in the Servo repository. (I haven’t checked all the transitive dependencies.) I considered using at some point, but I needed more than 64 bits: #13756.

I don’t really like EnumSet: it requires a custom implementation of CLike for every enum type it’s used with, even though this implementation should always be the same. This feels very fragile. I wouldn’t mind moving it from std into a Cargo package.

@Gankra
Copy link
Contributor

Gankra commented Dec 10, 2014

Ah, I see. Okay. I summon @aturon, lord-master of APIs.

@aturon
Copy link
Member

aturon commented Dec 11, 2014

Thanks for the PR, @tbu-!

I increasingly agree with @SimonSapin here: I don't think EnumSet as it stands is ready for prime time, and I don't want to sink a lot of effort into it just yet. Same with bitflags. Perhaps both should be moved out of tree? I'm not sure if there's an existing crate that would be a good place for them -- @gankro, would you want to iterate on this in your collects crate?

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2014

Yes I'm happy to adopt this and any other "imperfect" collections in collect-rs. I'm not sure how we want to handle such migrations.

@SimonSapin
Copy link
Contributor

I’ve exported the git history of the ascii module but I’m not sure it’s worth it. You could just include the hash of the Rust commit you copied from in the message for your commit that first imports something, and anyone interested in archaeology can follow the trail manually.

Is that what you meant by handling migrations?

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2014

I was more thinking about the actual political process. e.g. an RFC or a core-team meeting or what. But yeah having a nod to actual commit history would be nice.

@SimonSapin
Copy link
Contributor

Since removing things is a breaking change, probably an RFC. (And whatever the RFC approval process is these days − it’s changing but I haven’t read up on that yet.) But you can have a couple of related things in the same RFC.

@aturon
Copy link
Member

aturon commented Dec 11, 2014

@gankro Maybe your collections reform part 2 RFC should also include this migration. If you want to do that, we should talk about the other collections that we don't plan to stabilize in the near future and probably move them as well.

@aturon
Copy link
Member

aturon commented Dec 18, 2014

I'm going to close this PR now that EnumSet is on the way out of std. Maybe it should be retargetted to https://github.com/Gankro/collect-rs ?

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.

None yet

4 participants