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

Migrate argument parsing over to Clap #137

Closed
wants to merge 6 commits into from
Closed

Conversation

mazylol
Copy link
Contributor

@mazylol mazylol commented Oct 20, 2024

Clap is a robust argument parser and it's implementation makes the code a bit more understandable. I see this as a complete win. Additionally, I ran cargo update to get the lockfile modernized. Then I ran cargo-msrv to find the MSRV which then I added to the Cargo.toml + the README. Finally I bumped the version to 1.7.0.

image

@mazylol
Copy link
Contributor Author

mazylol commented Oct 20, 2024

It would appear that the --locked flag in the action shot me down 😭

@lunacookies
Copy link
Collaborator

Thanks for the contribution! We in fact used to use Clap but migrated to a custom argument parsing implementation in #130.

The rationale was that, for our use-case, Clap is quite bloated: at the time switching from Clap to hand-rolled argument parsing reduced our ARM64 macOS binary size from 1.4 MB to 1.0 MB. ARM64 instructions are four bytes each, so we were spending 100,000 instructions (and 29% of the whole binary) just on parsing arguments, which frankly is absurd. Last I checked Clap and its dependencies were around 200,000 lines of code, completely dwarfing not only the ~140 lines of our argument parsing, but also the entire pipes-rs codebase. Clap is so big, in fact, that the people who work on the Rust compiler use it as a compiler stress test.

On top of all that, #130 actually removed more lines than it added!

If you can think of anything that Clap could offer pipes-rs which would be non-trivial to write ourselves, though, then please do let us know. Same for if you spot any other rough edges in the argument parsing behavior :)

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