-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[ruby 2.4] Fixes for Integer#digits in PR #4375 #4490
Conversation
It passes all the following tests of TestInteger of MRI: * test_digits * test_digits_for_negative_numbers * test_digits_for_invalid_base_numbers * test_digits_for_non_integral_base_numbers * test_digits_for_non_numeric_base_argument
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 know some of these style issues were in the original, but you're active so you're the lucky one :-)
@Override | ||
public RubyArray digits(ThreadContext context, IRubyObject base) { | ||
|
||
long self = ((RubyFixnum)this).getLongValue(); |
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.
This is not necessary...this
is already known to be RubyFixnum.
*/ | ||
@Override | ||
public RubyArray digits(ThreadContext context, IRubyObject base) { | ||
BigInteger self = (this).getValue(); |
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.
You could just access the field here, or at least call it without the parens. I assume this is just a mistake converting the RubyFixnum-only logic.
public RubyArray digits(ThreadContext context, IRubyObject base) { | ||
BigInteger self = (this).getValue(); | ||
if (self.compareTo(new BigInteger("0")) == -1) { | ||
throw context.getRuntime().newMathDomainError("out of domain"); |
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.
Throw context.runtime into a local variable at the top and reuse it, in the style of other methods.
Thanks for the help! Squashed a few other style issues too. |
@headius Is there anything preventing this PR from being merged, or is it just a low priority for the moment? |
@whwilder great work - wanted to get |
I've split the implementation for Integer#digits amongst Fixnum and Bignum, which should fix the issues in #4375
ref #4293