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

Amend unknown bitflag contain check #6653

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Dec 3, 2024

Connections
I may have broken this in #6644 but im not fully certain. This should work and be clearer anyways.

Description
bitflag equality check may be truncating to known bits before comparing

Testing
intuition :| im not sure if we have test coverage of these failure cases. we may want to add some, im not sure how to though.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@atlv24 atlv24 requested a review from a team as a code owner December 3, 2024 06:55
@nical
Copy link
Contributor

nical commented Dec 3, 2024

(I am not opposed to this change but) I did a quick and dirty test and it looks like flags | Flags::all() != Flags::all() does what we want (it does not truncate the bits) with the latest version of the bitflags crate.

@atlv24
Copy link
Contributor Author

atlv24 commented Dec 3, 2024

oh, thanks! i just got worried, its good to know i didnt mess it up. tried reading the source and didnt find where Eq was implemented

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Dec 3, 2024

I feel like this could use a generic utility in an extension trait for all T: Flags, so the code would read flags.contains_invalid_bits() or smth

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Dec 3, 2024

Actually this should be able to be spelled flags.contains(T::all()), which is clear enough. Nope, it would have to be T::all().intersects(flags) or something - probably better to have it as a dedicated function so we don't have to remember which arcane bit thing we need to use.

@atlv24
Copy link
Contributor Author

atlv24 commented Dec 4, 2024

i tried the flags.intersects(!T::all()), it doesnt work. intersects is truncating i think

@atlv24
Copy link
Contributor Author

atlv24 commented Dec 4, 2024

bitflags/bitflags#425

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