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

Implement Int#{leading,trailing}_zeros_count #7520

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Mar 8, 2019

Closes #7517

Missing BigInt#leading_zeros_count implementation, which I have no idea how to tackle since LibGMP uses variable width integers.

@konovod
Copy link
Contributor

konovod commented Mar 8, 2019

BigInt#leading_zeros_count just doesn't make sense, so i think raising an exception is a correct implementation and raising should be reflected in specs.

@ysbaddaden
Copy link
Contributor

Or not implement it, so it will fail to compile.

@Sija Sija force-pushed the feature/int-zeros-count branch from f2f6e75 to ce85c2f Compare March 8, 2019 10:53
@Sija Sija marked this pull request as ready for review March 8, 2019 10:53
@yxhuvud
Copy link
Contributor

yxhuvud commented Mar 8, 2019

This will be useful for me immediately, but I wonder about the naming - wouldn't it make more sense to have it called count_leading_zeros? I guess it is a question of if it should be a order or a lookup of a property.

Are there any conventions around this? LLVM intrinsics clearly use the CLZ order.

Heck, or even just leading_zeros/ trailing_zeros

@Sija
Copy link
Contributor Author

Sija commented Mar 8, 2019

My favorite naming option would be leading_zeros / trailing_zeros (that's what Julia uses btw).

/cc @asterite @crystal-lang ?

@asterite
Copy link
Member

asterite commented Mar 8, 2019

My thought process:

  • in LLVM it's called count_leading_zeros because it's a function: count_leading_zeros(value) which can be read as "count leading zeros (of) value"
  • number.leading_zeros_count expressed that as an accessor/property
  • If I see "001000" and think leading_zeros I think about "00" so maybe I'd expect leading_zeros to return "00"

But I wouldn't mind ending up with count_leading_zeros or just leading_zeros if it won't be confusing.

@Sija
Copy link
Contributor Author

Sija commented Mar 8, 2019

Verb at the beginning in count_..._zeros reads badly for my taste (especially as it's an arg-less method), I'd prefer using a noun there, as in ..._zeros_count - if at all.

@@ -390,6 +390,14 @@ struct BigInt < Int
LibGMP.popcount(self)
end

def leading_zeros_count
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bew
Copy link
Contributor

bew commented Mar 13, 2019

I'd suggest binary_leading_zeros and binary_trailing_zeros to explicit the fact that it checks the binary representation

@Sija Sija force-pushed the feature/int-zeros-count branch from ce85c2f to 3170d9b Compare March 17, 2019 15:53
@j8r
Copy link
Contributor

j8r commented Mar 18, 2019

That's indeed pedantic, after reading the docs, there are Indexable#first, Indexable#last, Hash#first_key, Hash#first_value, Hash#last_key, Hash#last_value, XML::Node#first_element_child.
Wouldn't it be even more on par with other methods in the stdlib to name it first_binary_zeros and last_binary_zeros?

@konovod
Copy link
Contributor

konovod commented Mar 18, 2019

@j8r it would ne something like first_binary_one_index, because we seek for a first 1, not first 0. But that's not needed because "count leading\trailing zeros" is used in other languages and libraries.

@ysbaddaden
Copy link
Contributor

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 leading_zeros_count.

@Sija Sija force-pushed the feature/int-zeros-count branch 2 times, most recently from 3c7e154 to 57a359c Compare March 19, 2019 07:39
src/int.cr Outdated Show resolved Hide resolved
@Sija Sija force-pushed the feature/int-zeros-count branch from 57a359c to 82889c2 Compare March 19, 2019 21:20
@Sija Sija force-pushed the feature/int-zeros-count branch from 82889c2 to da330a8 Compare March 19, 2019 21:57
@asterite asterite merged commit a17a656 into crystal-lang:master Mar 20, 2019
@asterite asterite added this to the 0.28.0 milestone Mar 20, 2019
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 26, 2019
* 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)
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.

8 participants