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 rate::ilog64() #1466

Merged
merged 2 commits into from
Jul 26, 2019
Merged

Remove rate::ilog64() #1466

merged 2 commits into from
Jul 26, 2019

Conversation

rzumer
Copy link
Collaborator

@rzumer rzumer commented Jul 20, 2019

It is superseded by util.rs::ILog::ilog().

@rzumer rzumer force-pushed the ilog branch 3 times, most recently from 7b9c645 to d70ff73 Compare July 20, 2019 19:06
@lu-zero
Copy link
Collaborator

lu-zero commented Jul 20, 2019

It exists because we cannot have const-fn traits in stable and that could be the case for a while =/

@rzumer
Copy link
Collaborator Author

rzumer commented Jul 20, 2019

Right, so one of #1465 and #1466 can be chosen, or alternatively we could add const fn functions for all required PrimInts besides i64, not via a trait but in the same way that ilog64() works now.

@coveralls
Copy link
Collaborator

coveralls commented Jul 20, 2019

Coverage Status

Coverage decreased (-0.004%) to 81.303% when pulling 467a545 on rzumer:ilog into cdb51aa on xiph:master.

@rzumer
Copy link
Collaborator Author

rzumer commented Jul 26, 2019

After discussing with @lu-zero he feels indifferent about it (correct me if I'm wrong) and I prefer this solution, since less duplicated code seems a bigger benefit than constifying a non-critical function. I will squash and push after CI succeeds.

@lu-zero
Copy link
Collaborator

lu-zero commented Jul 26, 2019

It is better to have less lines of code around until the const-fn lands completely.

Keep in mind that it has an effect if the function arguments are constant themselves, otherwise it doesn't do anything :)

@rzumer rzumer merged commit 373e5d9 into xiph:master Jul 26, 2019
@rzumer rzumer mentioned this pull request Jul 26, 2019
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