Improve consistency/correctness of arg handling#1143
Conversation
Employ RubyProc.prepareArgs in a very central place, improving consistency of how args are handled between both yield and block.call. This involves migrating many calls to pass an IRubyObject[] of args rather than prematurely wrapping that array in a RubyArray (which makes it impossible for deeper calls to determine if they have one Array arg, or multiple args). As an added side effect, JavaInternalBlockBody now gets correct arg treatment, and can be used to get RubyEnumerable cleaner and fully rubyspec-compliant.
Improve consistency/correctness of arg handling
|
Hmmm, I merged this accidentally without reviewing it, and it was applied to master rather than 1.7. I wonder if you'd be able to make the same change against 1.7? And I need to review...bleh. |
|
Or...if we don't really need this fix in 1.7.x, ignore that part. |
|
Daniel, I reverted this not because I don't like stuff I am seeing but more because there are multiple logical changes in this commit. I am worried a behavior bug introduced by this patch will be difficult to bisect later (e.g. it will point to one big commit). I am also having a more difficult time reviewing this since I can see different things changing for different reasons and I am unsure if I am missing all the changes or not. Some suggestions would be to make things like newArrayNoCopy -> newArrayNoCopyLight be an individual commit. BlockCallback -> JavaInternalBlockBody as another, args[] as another. I think the name of the game is reducing these changes to be single logical changes. Also don't wild-card imports. We have very few of those in our code and it is not our style. You can use one PR if you want but I would prefer multiple PR's so that the conversation on each one is pretty focused. Please don't take this as a negative that I reverted. I just want it in smaller pieces :) |
|
You're absolutely right Tom. This grew organically from trying to centralize the arg prep, but now that the picture's complete, I should definitely slice it up for you. Look for those pulls soon... (And thanks for the note on imports. IDE code style settings: updated.) |
3 participants
In #928, I noted that args probably needed to be prepped pretty broadly. This pull accomplishes that, employing RubyProc.prepareArgs at the entry points in BlockBody.
This involves migrating many calls to pass an IRubyObject[] of args rather than prematurely wrapping that array in a RubyArray (which made it impossible for deeper calls to determine if they have one Array arg, or multiple args).
This change has a few nice side effects:
I know master Travis is red right now, but you can see a clean run of this branch (pre-rebase) against a fairly recent master here.
Also note: I know pull requests which refactor a bunch of critical path code are kind of a pain. This is the result of an experiment on my part, and the result seems solid so I thought I'd share, but I won't be put out in the least if we find reasons not to merge.