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

Add Int#bit_length #8924

Merged
merged 8 commits into from
Mar 22, 2020
Merged

Add Int#bit_length #8924

merged 8 commits into from
Mar 22, 2020

Conversation

asterite
Copy link
Member

Will be useful to implement https://forum.crystal-lang.org/t/integer-sqrt-in-stdlib . It's probably also a generally useful method. Also available in Ruby.

src/int.cr Outdated Show resolved Hide resolved
Co-Authored-By: Vladislav Zarakovsky <vlad.zar@gmail.com>
spec/std/int_spec.cr Outdated Show resolved Hide resolved
it "#bit_length" do
0.bit_length.should eq(0)
(1..100).each do |i|
i.bit_length.should eq(Math.log2(i).to_i + 1)
Copy link
Member

Choose a reason for hiding this comment

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

This is testing that the result of bit_length matches some mathematical formula. But it has about as much to do with the expected answers as the actual implementation of the method. Should you then have to write tests for this mathematical formula?
Tests shouldn't have logic, I'd suggest checking actual inputs.

Copy link
Member

Choose a reason for hiding this comment

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

Things like 0b1000 and 0b111.bit_length might look nice

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Done!

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks. But I think it still needs some checks specifically with "1+all-zeros" (like 0b1000) and "all-ones" (0b111) to catch off-by-one errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about now?

@asterite asterite added this to the 0.34.0 milestone Mar 22, 2020
@asterite asterite merged commit aed65fc into master Mar 22, 2020
@asterite asterite deleted the feature/int-bit-length branch March 22, 2020 13:41
carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Apr 29, 2020
* Add Int#bit_length

* Update src/int.cr

Co-Authored-By: Vladislav Zarakovsky <vlad.zar@gmail.com>

* Simpler Int#bit_length specs

* Spec for Int#bit_length negative values

* Let Int#bit_length work for BigInt

* More specs...

* Be nice with Windows

Co-authored-by: Vladislav Zarakovsky <vlad.zar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants