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

Drop the bitflags dependency #148

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Drop the bitflags dependency #148

merged 1 commit into from
Mar 5, 2024

Conversation

livingsilver94
Copy link
Contributor

This PR replaces the bitflags functionality with an in-house implementation.

I think we all agree on reducing the number of dependencies where it makes sense, and I also think bitflags is one dependency worth removing. It is based on a macro_rule which is able to escape the original Rust syntax (think of that : u8 syntax to specify that flags are bytes. That's a C++ notation!), making code a little confusing. macro_rules have too much freedom of toying with the code, IMO.

The new Flags struct is a type-safe and value-safe collection of flags. It is not possible to build an invalid collection of Flags. The idea is based on std::fs::OpenOptions.

Did you note that bits() private method? That's a micro-optimization. While I could implement the contains() method using pure boolean logic, with Compiler Explorer I noticed that that would introduce conditional jumps in the assembly. For the sake of the CPU pipeline, I chose to convert the single flags back into an integer for the comparison, thus avoiding jumps altogether.

@livingsilver94 livingsilver94 force-pushed the no-bitflags branch 2 times, most recently from e41ddec to 09223db Compare February 28, 2024 13:34
Copy link
Collaborator

@tarkah tarkah left a comment

Choose a reason for hiding this comment

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

Awesome! For me the win is not having LSP breakdown when doing go-to-definition on contains or any of the other implementations due to macro usage. Plus it's easier to reason about in general, I like.

One small comment around builder pattern.

@livingsilver94 livingsilver94 force-pushed the no-bitflags branch 2 times, most recently from b1e207a to 2bb9ef0 Compare February 28, 2024 18:12
@livingsilver94 livingsilver94 marked this pull request as draft February 28, 2024 18:12
@livingsilver94
Copy link
Contributor Author

Oops, made a mess with git history. Fixing.

@livingsilver94 livingsilver94 marked this pull request as ready for review February 28, 2024 18:24
@livingsilver94 livingsilver94 force-pushed the no-bitflags branch 6 times, most recently from d56b908 to 8f0e68e Compare March 5, 2024 09:09
@tarkah tarkah merged commit 389ce1c into main Mar 5, 2024
2 checks passed
@tarkah tarkah deleted the no-bitflags branch March 5, 2024 21:41
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.

2 participants