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

fix: compiling with all features #360

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

nelson137
Copy link
Contributor

Fix an error cause by compiling with all features: cargo build --all-features.

Make the conditional compilation on the const BITSET_EXP declarations mutually exclusive so that any combination of the keysize* features only ever results in a single instance.

Note that the keysize16 declaration does not exclude all other keysizes since it is the default. If multiple keysize features are enabled, including 16, then the declaration for keysize16 will be the only one. If multiple keysize features are enabled, not including 16, then there will be no declarations, causing a compile error. This is fine since users should only enable one of these features at a time.

error[E0428]: the name `BITSET_EXP` is defined multiple times
  --> framework_crates/bones_ecs/src/bitset.rs:18:1
   |
16 | const BITSET_EXP: u32 = 16;
   | --------------------------- previous definition of the value `BITSET_EXP` here
17 | #[cfg(feature = "keysize20")]
18 | const BITSET_EXP: u32 = 20;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `BITSET_EXP` redefined here
   |
   = note: `BITSET_EXP` must be defined only once in the value namespace of this module

error[E0428]: the name `BITSET_EXP` is defined multiple times
  --> framework_crates/bones_ecs/src/bitset.rs:20:1
   |
16 | const BITSET_EXP: u32 = 16;
   | --------------------------- previous definition of the value `BITSET_EXP` here
...
20 | const BITSET_EXP: u32 = 24;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `BITSET_EXP` redefined here
   |
   = note: `BITSET_EXP` must be defined only once in the value namespace of this module

error[E0428]: the name `BITSET_EXP` is defined multiple times
  --> framework_crates/bones_ecs/src/bitset.rs:22:1
   |
16 | const BITSET_EXP: u32 = 16;
   | --------------------------- previous definition of the value `BITSET_EXP` here
...
22 | const BITSET_EXP: u32 = 32;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `BITSET_EXP` redefined here
   |
   = note: `BITSET_EXP` must be defined only once in the value namespace of this module

@MaxCWhitehead
Copy link
Collaborator

MaxCWhitehead commented Mar 29, 2024

This sounds alright to me, I think being able to compile with all features might be helpful to catch issues. @zicklag any thoughts / concerns?

I wish there was a cleaner way of exposing this constant to crate user instead of feature flags, but I couldn't think of anything (or anything that sounds better lol).

@zicklag
Copy link
Member

zicklag commented Mar 29, 2024

This looks good to me. 👍️ In the long term I think we're going to switch to Roaring ( compressed ) bitmaps, which can have better performance and don't need to be configured for memory usage.

@nelson137
Copy link
Contributor Author

@zicklag do you mind creating an issue for that work?

@MaxCWhitehead MaxCWhitehead added this pull request to the merge queue Mar 29, 2024
Merged via the queue into fishfolk:main with commit e52ca65 Mar 29, 2024
9 of 10 checks passed
@zicklag
Copy link
Member

zicklag commented Mar 29, 2024

@nelson137 Here's an issue for the bitmap: #363.

@nelson137 nelson137 deleted the fix/overlapping-features branch March 29, 2024 20:24
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.

3 participants