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

Rename len() to count() to reflect its performance implication #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

upsuper
Copy link

@upsuper upsuper commented Aug 29, 2020

In Rust, len() (as a noun) is commonly used to refer to something which can be figured out in constant time, usually reading a field or so, while the function here apparently is not just reading some field, but instead iterating through the internal vector and add up the number of set bits. For non-constant time operations, it should be preferred to use a verb name to indicate that it may not be cheap. I think the convention is to use count() for this scenario.

I also bumped the minor version as renaming a function is a breaking change. Let me know if you prefer keeping len() but having it deprecated so that we don't do breaking change for this.

@pczarn
Copy link
Contributor

pczarn commented Jun 30, 2024

I would prefer to deprecate len just so users know about this change, and the transition is easier.

@upsuper
Copy link
Author

upsuper commented Jun 30, 2024

Okay, I just updated it to mark len deprecated rather than replacing it completely.

@pczarn
Copy link
Contributor

pczarn commented Jun 30, 2024

Thanks. I will merge within a few days and release with the next minor breaking version, "just in case", even if there is no need to bump minor (is that how semver works for version 0.x?)

@pczarn
Copy link
Contributor

pczarn commented Jun 30, 2024

I will merge this after releasing 0.6.1 with recent bug fix

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