Skip to content

Delete EnumSet and remove its final use in rustc#26705

Closed
jroesch wants to merge 5 commits into
rust-lang:masterfrom
jroesch:remove-enum-set
Closed

Delete EnumSet and remove its final use in rustc#26705
jroesch wants to merge 5 commits into
rust-lang:masterfrom
jroesch:remove-enum-set

Conversation

@jroesch

@jroesch jroesch commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

This commit adds an implementation of BuiltinBounds that does not depend on external data structures and removes EnumSet in the standard collections library. We should probably have both a person from the compiler team and libs team look this one over.

r? @gankro

@Gankra

Gankra commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

To be clear: EnumSet is already deprecated from libstd, and isn't re-exported there. As such you can just delete it. It's an internal implementation detail that was kept in exclusively for rustc's consumption.

Comment thread src/librustc/middle/ty.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.bits |= match ... ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just cast the BuiltinBound to an integer. So

pub fn insert(&mut self, bound) {
    self.bits |= (1 << (bound as u8));
}

pub fn contains(& self, bound) {
    ((self.bits >> (bound as u8)) & 1) == 1
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point some of these are probably just my functional programmer side coming through ;)

@Gankra

Gankra commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

The collection logic seems legit.

@jroesch jroesch changed the title Deprecate EnumSet and remove its final use in rustc Delete EnumSet and remove its final use in rustc Jul 1, 2015
@jroesch jroesch force-pushed the remove-enum-set branch from fdfd38b to 2c8ab28 Compare July 2, 2015 02:31
@Gankra

Gankra commented Jul 5, 2015

Copy link
Copy Markdown
Contributor

@jroesch What's the status on this; do you need any more review? r=me if this is good to go.

@jroesch

jroesch commented Jul 6, 2015

Copy link
Copy Markdown
Contributor Author

@gankro I didn't receive any negative feedback the second time so I assume that we are good to go.

@Gankra

Gankra commented Jul 7, 2015

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jul 7, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit 2c8ab28 has been approved by Gankro

@bors

bors commented Jul 8, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 2c8ab28 with merge 057a1d7...

@bors

bors commented Jul 8, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-mac-64-nopt-t

@jroesch

jroesch commented Jul 8, 2015

Copy link
Copy Markdown
Contributor Author

I didn't run the full test suite since this wasn't a huge change, but one test was still mentioning CLike 😢

@alexcrichton

Copy link
Copy Markdown
Member

@bors: r=Gankro f4ff8ef

@bors

bors commented Jul 8, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit f4ff8ef with merge ef9570c...

@bors

bors commented Jul 8, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-mac-64-nopt-t

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this almost exactly the same as bitflags?

@steveklabnik

Copy link
Copy Markdown
Contributor

@jroesch seems like a straightforward failure here

@tamird

tamird commented Jul 19, 2015

Copy link
Copy Markdown
Contributor

metaquestion: what is the difference between EnumSet/BitSet and bitflags?

@arielb1

arielb1 commented Jul 19, 2015

Copy link
Copy Markdown
Contributor

@tamird

One is a macro which declares a statically-sized set, the other is a dynamically-resizing set.

@Gankra

Gankra commented Jul 19, 2015

Copy link
Copy Markdown
Contributor

@arielb1 EnumSet also has a statically known size (number of variants).

@tamird

tamird commented Jul 20, 2015

Copy link
Copy Markdown
Contributor

Seems like we can avoid introducing BitSet and just use bitflags!

@Gankra

Gankra commented Jul 20, 2015

Copy link
Copy Markdown
Contributor

This sound plausible, yes. @jroesch what's the status?

@jroesch

jroesch commented Jul 20, 2015

Copy link
Copy Markdown
Contributor Author

@gankro I had forgot this PR was still hanging around. I fixed the test failure, but we could just use bitflags if you don't foresee any problems.

@bors

bors commented Jul 26, 2015

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #26870) made this pull request unmergeable. Please resolve the merge conflicts.

@jroesch

jroesch commented Jul 26, 2015

Copy link
Copy Markdown
Contributor Author

I've decided I'm just going to remove BuiltinBounds as per @eddyb 's suggestion will follow up on this PR.

@Gankra

Gankra commented Jul 27, 2015

Copy link
Copy Markdown
Contributor

I'm just going to close this PR, since it sounds like the new one will just be fresh work.

Good luck!

@Gankra Gankra closed this Jul 27, 2015
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.

7 participants