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

inline btoi to reduce compile times #1292

Merged
merged 5 commits into from
Feb 15, 2024
Merged

inline btoi to reduce compile times #1292

merged 5 commits into from
Feb 15, 2024

Conversation

benmkw
Copy link
Contributor

@benmkw benmkw commented Feb 15, 2024

This PR inlines the calls to btoi into gix-utils to reduce compile times (removes num-traits as well).
Compared to the previous diff I extracted basic tests from the doc comments and created a macro to make it easier to add new types/ reduce noise.
If needed I can squash the commits.
I have https://github.com/tamasfe/taplo installed and it reformatted the touched Cargo.toml files, I can revert that as well if wanted.

ref discussion in #729 (comment)

@Byron
Copy link
Member

Byron commented Feb 15, 2024

Thanks a lot for your tremendous help with this!

I will merge once CI is green after my refactor round.

Ok(result)
}

/// minimal subset of traits used by btoi_radix and btou_radix
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// minimal subset of traits used by btoi_radix and btou_radix
/// minimal subset of traits used by [`to_signed_with_radix `] and [`to_signed_with_radix`]

or however it is that doclinks (or just generally improve this doc?)

Copy link
Member

Choose a reason for hiding this comment

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

That's a great catch, thanks for the suggestion.
I didn't apply the fix because it uses to_signed_with_radix twice, but applied the intended fix locally and force-pushed.

- fix doc tests and enable them
- move all assertions onto integration test level and out of the module
- module documentation with credits section.
- use rustdoc feature to link other methods
- renamed functions to be more obvious
@Byron Byron merged commit 5fc379d into GitoxideLabs:main Feb 15, 2024
18 checks passed
Comment on lines +275 to +276
///
fn zero() -> Self;
Copy link
Contributor

Choose a reason for hiding this comment

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

have you considered changing this to

Suggested change
///
fn zero() -> Self;
/// the 0 value of this type
const ZERO: Self;

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh apologies for doing this post-merge, the browser hadn't refreshed

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see! Some of these could probably be const, maybe const-fn in traits is allowed too?

In any case, if you would like to submit a PR, that would be very welcome.

Copy link
Contributor Author

@benmkw benmkw Feb 16, 2024

Choose a reason for hiding this comment

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

I just took this from num-traits with minor changes (changed &x parameters of checked_* to pass by value cause its only used for small copy types here). Making it an associated const seems reasonable to me as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realized later in the evening that it was likely to do with MSRV that such consts were not used, but maybe that is 'too conservative'?

I'll see about a PR if that's ok with benmkw :)

@benmkw benmkw mentioned this pull request Feb 16, 2024
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