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

Implemented optional buffer argument for Array#pack #4502

Merged
merged 3 commits into from
Feb 27, 2017
Merged

Implemented optional buffer argument for Array#pack #4502

merged 3 commits into from
Feb 27, 2017

Conversation

whwilder
Copy link
Contributor

ref #4293

public RubyString pack(ThreadContext context, IRubyObject obj, IRubyObject maybeOpts) {
Ruby runtime = context.runtime;
IRubyObject opts = ArgsUtil.getOptionsArg(runtime, maybeOpts);
RubyString str = RubyString.newEmptyString(runtime);
Copy link
Member

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;
Copy link
Member

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){
Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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!

@kares kares merged commit caa22d5 into jruby:ruby-2.4 Feb 27, 2017
@enebo enebo added this to the JRuby 9.2.0.0 milestone Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants