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

Conversation

Projects
None yet
3 participants
@whwilder
Contributor

whwilder commented Feb 23, 2017

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);

This comment has been minimized.

@kares

kares Feb 24, 2017

Member

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;

This comment has been minimized.

@kares

kares Feb 24, 2017

Member

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){

This comment has been minimized.

@kares

kares Feb 25, 2017

Member

why is this introduced here? is it due the obj.convertToString() removal? (would keep it than)

This comment has been minimized.

@whwilder

whwilder Feb 25, 2017

Contributor

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.

This comment has been minimized.

@kares

kares Feb 25, 2017

Member

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@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