-
-
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
Implemented optional buffer argument for Array#pack #4502
Conversation
public RubyString pack(ThreadContext context, IRubyObject obj, IRubyObject maybeOpts) { | ||
Ruby runtime = context.runtime; | ||
IRubyObject opts = ArgsUtil.getOptionsArg(runtime, maybeOpts); | ||
RubyString str = RubyString.newEmptyString(runtime); |
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.
preferably use RubyString.newString()
the newEmptyString is for cases such as return ''
if (!opts.isNil()){ | ||
IRubyObject buffer = ((RubyHash) opts).fastARef(runtime.newSymbol("buffer")); | ||
if (buffer != null) { | ||
str = (RubyString) buffer; |
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.
maybe the RubyString
allocation (above) can be delayed until its sure a buffer is not provided?
} catch (ArrayIndexOutOfBoundsException e) { | ||
throw concurrentModification(context.runtime, e); | ||
} catch (ClassCastException e){ |
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.
why is this introduced here? is it due the obj.convertToString()
removal? (would keep it than)
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.
In my original implementation I hadn't considered what would happen if the buffer were nil or some other class besides String. MRI Ruby throws a type error, and this seemed to be the most straightforward way of mirroring that behavior.
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.
buffer.convertToString()
will do that for you - raising a TypeError
e.g. on nil
... ClassCastException
isn't very bad but theoretically might happen from some other place
if you could update to get rid of it that would be great ... the rest of the code is great merge material!
ref #4293