-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement Int#{leading,trailing}_zeros_count #7520
Conversation
|
Or not implement it, so it will fail to compile. |
f2f6e75
to
ce85c2f
Compare
This will be useful for me immediately, but I wonder about the naming - wouldn't it make more sense to have it called Are there any conventions around this? LLVM intrinsics clearly use the CLZ order. Heck, or even just |
My favorite naming option would be /cc @asterite @crystal-lang ? |
My thought process:
But I wouldn't mind ending up with |
Verb at the beginning in |
src/big/big_int.cr
Outdated
@@ -390,6 +390,14 @@ struct BigInt < Int | |||
LibGMP.popcount(self) | |||
end | |||
|
|||
def leading_zeros_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to simply not declare this method on BigInt
. It does only apply to fixed-width integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this is just repeating compiler's work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's impossible since it's declared as abstract
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it shouldn't be an abstract method on Int
, because the concept of leading zeros doesn't apply to all integer types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@straight-shoota so what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It declares a method that makes no sense, and duplicates what the compiler already does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but having it declared directly on Int
as abstract
makes it possible to use it for all Int
types without duck typing or any other checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the other way around: if we ever support holding a variable of type Int, then invoking a method in it will have the compiler type all methods of all subclasses. It will find the one in BigInt and the compilation will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I've forgot about that case, I'll remove it then. Is it ok to just comment it out, to keep it more visible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I'd suggest |
ce85c2f
to
3170d9b
Compare
That's indeed pedantic, after reading the docs, there are |
@j8r it would ne something like |
The feature is very specific, already known by some name. Giving it another name will only make it more difficult to find when it's needed. Let's just use |
3c7e154
to
57a359c
Compare
57a359c
to
82889c2
Compare
82889c2
to
da330a8
Compare
* upstream/master: fix example codes (2019-03) (crystal-lang#7569) Change bsearch's `/ 2` to `>> 1` for faster performance (crystal-lang#7531) Don't include private constants in the docs (crystal-lang#7575) Add missing `require` statements for the API (A-N) (crystal-lang#7564) Implement Int#{leading,trailing}_zeros_count (crystal-lang#7520)
Closes #7517
Missing
BigInt#leading_zeros_count
implementation, which I have no idea how to tackle sinceLibGMP
uses variable width integers.