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

Implement set complement and universe for bitflags #14009

Merged
merged 1 commit into from
May 14, 2014
Merged

Implement set complement and universe for bitflags #14009

merged 1 commit into from
May 14, 2014

Conversation

jcmoyer
Copy link
Contributor

@jcmoyer jcmoyer commented May 7, 2014

I feel that this is a very vital, missing piece of functionality. This adds on to #13072.

Only bits used in the definition of the bitflag are considered for the universe set. This is a bit safer than simply inverting all of the bits in the wrapped value.

bitflags!(flags Flags: u32 {
    FlagA       = 0x00000001,
    FlagB       = 0x00000010,
    FlagC       = 0x00000100,
    FlagABC     = FlagA.bits
                | FlagB.bits
                | FlagC.bits
})

...

// `Not` implements set complement
assert!(!(FlagB | FlagC) == FlagA);
// `all` and `is_all` are the inverses of `empty` and `is_empty`
assert!(Flags::all() - FlagA == !FlagA);
assert!(FlagABC.is_all());

@jcmoyer
Copy link
Contributor Author

jcmoyer commented May 7, 2014

The Not trait (no matter how you qualify it) doesn't seem to be available inside of libstd macros. I have no idea what is going on here. If I remove the attribute forwarding part of the macro this error goes away and a parse error appears in a totally unrelated module. Might be a macro_rules related bug.

The code compiles and all tests pass when the module is used standalone.

@alexcrichton
Copy link
Member

Is there precedent for a universe method? I'm not sure I've heard of that before, but it does seem useful!

This also has a travis failure which will need to get fixed, it looks like the Not trait needs to be imported in std::io

@sfackler
Copy link
Member

sfackler commented May 7, 2014

Universe is a pretty standard term in set theory. I'm not sure if there's a different term used for this kind of thing, though.

@jcmoyer
Copy link
Contributor Author

jcmoyer commented May 8, 2014

@alexcrichton: D'oh! I didn't realize the macro was already being used elsewhere in the stdlib. I'll fix this immediately. It was a late night. :)

My decision to use the term universe was based on my discussion with thestinger in #13072. After a bit of research I came across this and ran with it. I'm not aware of it being used in other programming languages.

@sfackler
Copy link
Member

sfackler commented May 8, 2014

One possible alternative to universe is all, which is what Java's EnumSet uses: http://docs.oracle.com/javase/7/docs/api/java/util/EnumSet.html#allOf(java.lang.Class)

@@ -122,6 +126,11 @@ macro_rules! bitflags(
$BitFlags { bits: 0 }
}

/// Returns the set containing all flags.
Copy link
Member

Choose a reason for hiding this comment

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

A common use case would also contain masks. If so, would this be correct? We could add another section to the macro perhaps) eg. flags { ... } masks { ... }

Copy link
Member

Choose a reason for hiding this comment

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

According to @alexcrichton, masks are usually subsets of the universal set, so this shouldn't be a problem. I think we should mention though in the macro's comment that the universe is not statically checked (some C bindings are probably weird like that), and in that case it is up to the user to define those outside the macro, eg. static NotInUniverse: Flags = Flags { bits: ??? }.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could auto-generate tests for the validity of the universe and complement functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bjz: Could you give me an example where universe or complement would return an invalid value? How would you test for validity?

@brendanzab
Copy link
Member

@alexcrichton The universal set is a common term in set theory. As long as it is documented, I think it is fine.

@emberian
Copy link
Member

emberian commented May 8, 2014

I think the name "all" would be better. It's certainly what I would expect, and reads more sensibly without any math background: "all - the set of all flags", "is_all - returns true if every flag is set"

@brendanzab
Copy link
Member

Yeah, all is probably a better name. Less fun though!

@aturon
Copy link
Member

aturon commented May 8, 2014

This is great, and a clever implementation! I'd vote for the less fun, but more obvious all.

@jcmoyer
Copy link
Contributor Author

jcmoyer commented May 8, 2014

Renamed universe -> all.

@brendanzab
Copy link
Member

Might need to rebase this. Then I can r+.

@jcmoyer
Copy link
Contributor Author

jcmoyer commented May 14, 2014

@bjz: Done.

@alexcrichton
Copy link
Member

Could you squash the two commits together, other than that, this looks good to go!

@jcmoyer
Copy link
Contributor Author

jcmoyer commented May 14, 2014

@alexcrichton: sure thing!

bors added a commit that referenced this pull request May 14, 2014
I feel that this is a very vital, missing piece of functionality. This adds on to #13072.

Only bits used in the definition of the bitflag are considered for the universe set. This is a bit safer than simply inverting all of the bits in the wrapped value.

```rust
bitflags!(flags Flags: u32 {
    FlagA       = 0x00000001,
    FlagB       = 0x00000010,
    FlagC       = 0x00000100,
    FlagABC     = FlagA.bits
                | FlagB.bits
                | FlagC.bits
})

...

// `Not` implements set complement
assert!(!(FlagB | FlagC) == FlagA);
// `all` and `is_all` are the inverses of `empty` and `is_empty`
assert!(Flags::all() - FlagA == !FlagA);
assert!(FlagABC.is_all());
```
@bors bors closed this May 14, 2014
@bors bors merged commit 1595885 into rust-lang:master May 14, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2023
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