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

Remove dependency on num_enum. #786

Merged
merged 8 commits into from
May 21, 2024

Conversation

rbartlensky
Copy link
Contributor

Hello!

I am using libbpf-rs in one of my personal projects and I noticed that I was pulling in toml-edit via libbpf-rs which accounted for 1.3 seconds of my total build time. With the help of cargo tree I realized that num_enum is pulling in that dependency (and many others!) and I decided to try to remove it from libbpf-rs completely. Let me know what you think!

cargo udeps also noticed some extra things that can be removed, so I went ahead and removed those.

These were suggested by `cargo udeps`.
Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal! I am in favor of removing these dependencies, I am just not sure about the current implementation hardcoding the values. Can we instead rely on the symbolic values, ala:

--- libbpf-rs/src/btf/mod.rs
+++ libbpf-rs/src/btf/mod.rs
@@ -96,7 +96,7 @@ impl TryFrom<u32> for BtfKind {
         use BtfKind::*;

         Ok(match value {
-            0 => Void,
+            x if x == Void as u32 => Void,

29 => TaskStorage,
30 => BloomFilter,
31 => UserRingBuf,
u32::MAX => Unknown,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this case? What purpose does it serve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

consistency with the "old" implementation, but to be fair it's useless, so I removed it

@rbartlensky
Copy link
Contributor Author

Thanks for the proposal! I am in favor of removing these dependencies, I am just not sure about the current implementation hardcoding the values. Can we instead rely on the symbolic values, ala:

--- libbpf-rs/src/btf/mod.rs
+++ libbpf-rs/src/btf/mod.rs
@@ -96,7 +96,7 @@ impl TryFrom<u32> for BtfKind {
         use BtfKind::*;

         Ok(match value {
-            0 => Void,
+            x if x == Void as u32 => Void,

I think that's even better! I made the changes suggested.

@rbartlensky
Copy link
Contributor Author

@danielocfb Thank you for taking the time to review! When you're happy with the PR I will squash the fixups away.

Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

The changes look great now! Feel free to squash and we can go ahead with the merge. Thanks!

Tests ensure that we didn't mess anything up.
Tests ensure that we didn't mess anything up.
Tests ensure that we didn't mess anything up.
Tests ensure that we didn't mess anything up.
Tests ensure that we didn't mess anything up.
These changes were suggested by clippy.
@rbartlensky
Copy link
Contributor Author

The changes look great now! Feel free to squash and we can go ahead with the merge. Thanks!

Squashed, and ready to go -- thank you!

@danielocfb danielocfb merged commit 200048e into libbpf:master May 21, 2024
12 checks passed
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