Implemented optional buffer argument for Array#pack#4502
Implemented optional buffer argument for Array#pack#4502kares merged 3 commits intojruby:ruby-2.4from whwilder:ruby-2.4-array-pack
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.
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.
maybe the RubyString allocation (above) can be delayed until its sure a buffer is not provided?
| return str.append(Pack.pack(context, context.runtime, this, iFmt)); | ||
| } catch (ArrayIndexOutOfBoundsException e) { | ||
| throw concurrentModification(context.runtime, e); | ||
| } catch (ClassCastException e){ |
There was a problem hiding this comment.
why is this introduced here? is it due the obj.convertToString() removal? (would keep it than)
There was a problem hiding this comment.
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.
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!
3 participants
ref #4293