-
-
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
[4688] add context and self to SizeFn #4714
Conversation
@@ -1226,7 +1226,7 @@ protected SizeFn enumLengthFn() { | |||
final RubyArray self = this; | |||
return new SizeFn() { | |||
@Override | |||
public IRubyObject size(IRubyObject[] args) { | |||
public IRubyObject size(IRubyObject[] args, ThreadContext context, IRubyObject self1) { |
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.
think it would be better to follow the 'general' order convention: (context, self, args)
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.
Thank you ! I will follow the same
@@ -3793,7 +3793,7 @@ private SizeFn combinationSize(final ThreadContext context) { | |||
final RubyArray self = this; | |||
return new SizeFn() { | |||
@Override | |||
public IRubyObject size(IRubyObject[] args) { | |||
public IRubyObject size(IRubyObject[] args, ThreadContext context, IRubyObject self1) { |
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.
self1
maybe just self
...
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.
Sure
import org.jruby.runtime.Helpers; | ||
import org.jruby.runtime.JavaInternalBlockBody; | ||
import org.jruby.runtime.JavaSites; | ||
import org.jruby.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.
✂️ I hate when IDEA does that ... already tried forcing it not to do so twice 😞
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.
I will remove this
@kares I will implement the changes as per your review. Thank you very much. |
e759be1
to
f0461eb
Compare
@kares I am done with the changes. Now can u take a look ? Ty |
public IRubyObject size(IRubyObject[] args) { | ||
return self.size(context); | ||
public IRubyObject size(ThreadContext context, IRubyObject self, IRubyObject[] args) { | ||
return this.size(context, self, args); |
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.
will recurse infinitely and thus fail
... I see why you had self1
now - makes sense to have a different name here
if I am reading this right? could have used a different name e.g. recv
if it conflicts (doesn't matter much though)
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.
Yeah that's why I had to use self1
. recv
makes it much better i will go ahead with that then :)
0cbaee7
to
ed11893
Compare
@kares I have renamed all instances of |
public IRubyObject size(IRubyObject[] args) { | ||
return self.size(context); | ||
public IRubyObject size(ThreadContext context, IRubyObject recv, IRubyObject[] args) { | ||
return this.size(context, recv, args); |
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.
this is still doing smt else as before, isn't it? (calling this's method instead of RubyEnumerator#size
)
@kares Sorry I missed that one. It's taken care of now. Sorry for wasting your time. This PR should be good now. |
thanks, the StackOverflow regression is still there: https://travis-ci.org/jruby/jruby/jobs/255126453 (view "Raw Log" and scroll to the end). but we can fix that for you later once we're actually looking at the code from an IDE :) |
@anukin Sorry I think I probably messed something up when I tried to update a merge conflict. Can you update this PR? |
Part of this – the addition of ThreadContext to the SizeFn interface – was done in #5958. The remaining changes from this PR have been reworked and remerged with master in #6321. Those changes alone are not sufficient to resolve #4688 but at least bring the "self" object through so that the remaining work can happen (as described in #6321). Original changeset retains @anukin attribution. 👍 |
@headius Can u take a look at this and suggest some changes if necessary? It's a solution for #4688