Consider using a bitflags type instead of HashSet

[de-vri-es]

Since we were talking about breaking changes, would it make sense to switch to a bitflags type for the seals instead of using a HashSet? It seems a bit overkill for at most 5 distinct values :)

[de-vri-es]

Personally, I prefer enumflags2 over bitflags. It looks more like ideomatic Rust.

[nagisa]

I don't like HashSet, but I don't particularly love bitflags either. Its definitely way better though.

[de-vri-es]

Yeah, me neither. I do like enumflags2 though. Have you had a look? (I realize you may not have seen my second message yet, considering the timing.)

[nagisa]

Exposing or requiring 3rd party crates to interact with the APIs in this crate doesn’t sound very nice, which enumflags2 would require AFAICT.

The reason I don't like bitflags too much is mostly because the API surface of the generated type is somewhat more broad than I would be comfortable with exposing. For instance it implements Ord and PartialOrd but that doesn’t make sense for bitflags, does it?

I think the best approach here would likely be a manual implementation to be honest, that somewhat mimics what bitflags does, but significantly more conservative in the traits that it implements.

So something along the lines of, perhaps:

#[derive(Copy, Clone, PartialEq, Eq)]
pub struct Seals(u32);

impl Seals {
    pub const SEAL_SHRINK: Seals = Seals(libc::F_SEAL_SHRINK);
    // etc.
    fn contains(&self, Seal) -> bool;
}

impl BitOr<Seals> for Seals { ... }
impl BitOrAssign<Seals> for Seals { ... }
impl IntoIterator for Seals { ... }
impl Debug for Seals { ... }

[de-vri-es]

There could be a pub type Seals = enumflags2::BitFlags<Seal>. That would remove the need for dependent crates to also depend directly on enumflags2.

However, I do like short dependency trees, and for that a manual implementation is best.

[de-vri-es]

I started working on this, but I think we do want almost the entire interface generated by bitflags. The type is not just for adding seals, there is also a function to retrieve the seals. Presumably, the user want to inspect the flags at that point.

For that, the other operators and functions added by bitflags are very useful. So maybe just using bitflags is the most pragmatic thing to do here.

I'd be willing to implement all of those manually too, just to avoid the external dependency. But then I'd like to achieve consensus on adding them first :)

[lucab]

I'm certainly biased, but I don't see the value behind this proposal. I can't foresee this logic sitting in any kind of hot-path performance-wise, so the gain on optimization is minimal. And I'm not really fond of old-school C bit-manipulation of integers, because the real semantics to model here is "a set of unique flags" not "boolean arithmetic modulo N".

We define the known flags, and then everybody already knows how to use the container from stdlib. That makes it direct to perform logic operations like is_disjoint/is_superset/etc without going through mind-twisting binary math.

[de-vri-es]

Well, bitflags also defines named operations: https://docs.rs/bitflags/1.2.1/bitflags/#methods-1. You can write a.intersect(b).is_empty() and a.contains(b).

We could even add other methods manually if we want.

In general, I don't think it's easy to predict how this will be used by all future applications. But it's mainly just that a HashMap for four bitflags feels like using a high-pressure cleaner to do the dishes.

[hellow554]

I found this library today and this was exactly my first thought... why a hashset? I have to write 3-ish lines and instatiate a whole container to tell the library what seals I want.

Here's what petgraph does:

Dot::with_config(&graph, &[Config::EdgeNoLabel]);

which is nice IMHO :)

Maybe that's worth a discussion? No external dependencies and a short expression of what I want.

[hgaiser]

@hellow554 looks like @de-vri-es wrote his own implementation: memfile-rs with minimal dependencies and its own Seal implementation. Might be worth a look too?

[hellow554]

Sure, you can write your own bitflags-like implementation. But "my" approach uses a slice of enums, which does not require such hacks :)

[de-vri-es]

Well, I really don't see an issue with this:

file.add_seals(Seal::Write | Seal::Shrink | Seal::Grow)?;

The bitmask computation is probably entirely optimized away by the compiler. That probably wouldn't happen with the list version.

To extend on my previous analogy a bit: A list of seals is maybe not a high pressure cleaner anymore, but it's still like using a dishwasher to clean two teaspoons (that you used to stir tea with).

[hellow554]

I'm not saying, that your approach ist bad, I like it.

But instead, I wanted to show off a different approach, because nagisa and lucab didn't seem to like to have a crate in a public interface and I can understand that.

As said: just a different approach to the same problem ;)

[de-vri-es]

Well, I do think a slice is already a lot better than a HashSet, thats for sure :p