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.

Sign up for GitHub

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

Jump to bottom

[4688] add context and self to SizeFn #4714

Closed
wants to merge 3 commits into from
Closed

[4688] add context and self to SizeFn #4714

wants to merge 3 commits into from
+54 −44

Conversation

anukin
Copy link
Contributor

@anukin anukin commented Jul 12, 2017

@headius Can u take a look at this and suggest some changes if necessary? It's a solution for #4688

@anukin anukin mentioned this pull request Jul 12, 2017
Closed
kares
kares reviewed Jul 12, 2017
View reviewed changes
core/src/main/java/org/jruby/RubyArray.java Outdated
@@ -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) {
Copy link
Member

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)

Copy link
Contributor Author

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

kares
kares reviewed Jul 12, 2017
View reviewed changes
core/src/main/java/org/jruby/RubyArray.java Outdated
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self1 maybe just self ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

kares
kares reviewed Jul 12, 2017
View reviewed changes
core/src/main/java/org/jruby/RubyEnumerable.java Outdated
import org.jruby.runtime.Helpers;
import org.jruby.runtime.JavaInternalBlockBody;
import org.jruby.runtime.JavaSites;
import org.jruby.runtime.*;
Copy link
Member

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 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove this

@anukin
Copy link
Contributor Author

anukin commented Jul 13, 2017

@kares I will implement the changes as per your review. Thank you very much.

@anukin anukin force-pushed the master branch 2 times, most recently from e759be1 to f0461eb Compare July 18, 2017 07:24
@anukin
Copy link
Contributor Author

anukin commented Jul 18, 2017

@kares I am done with the changes. Now can u take a look ? Ty

kares
kares reviewed Jul 18, 2017
View reviewed changes
core/src/main/java/org/jruby/RubyEnumerator.java Outdated
public IRubyObject size(IRubyObject[] args) {
return self.size(context);
public IRubyObject size(ThreadContext context, IRubyObject self, IRubyObject[] args) {
return this.size(context, self, args);
Copy link
Member

@kares kares Jul 18, 2017 •
edited
Loading

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)

Copy link
Contributor Author

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

@anukin anukin force-pushed the master branch 2 times, most recently from 0cbaee7 to ed11893 Compare July 18, 2017 10:29
@anukin
Copy link
Contributor Author

anukin commented Jul 18, 2017

@kares I have renamed all instances of self1 to recv. Should be good now :)

@enebo
Copy link
Member

enebo commented Jul 18, 2017

@anukin we typically use recv for JRubyMethod which are for modules but the intent is very clear here and it is a good analogue to self in this case. self1 bugged me and no doubt was why @kares originally complained.

kares
kares requested changes Jul 18, 2017
View reviewed changes
core/src/main/java/org/jruby/RubyEnumerator.java Outdated
public IRubyObject size(IRubyObject[] args) {
return self.size(context);
public IRubyObject size(ThreadContext context, IRubyObject recv, IRubyObject[] args) {
return this.size(context, recv, args);
Copy link
Member

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)

@anukin
Copy link
Contributor Author

anukin commented Jul 19, 2017

@kares Sorry I missed that one. It's taken care of now. Sorry for wasting your time.
@enebo self1 was bugging me as well. But i had seen some context1 in the codebase. So i was thinking self1 might be the best alternative. Seems there is a better one. Ty for clarifying that :)

This PR should be good now.

@kares
Copy link
Member

kares commented Jul 19, 2017 •
edited
Loading

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

@kares kares changed the title [4688]add context and self to SizeFn [4688] add context and self to SizeFn Jul 19, 2017
@enebo
Copy link
Member

enebo commented Jan 29, 2018

@anukin Sorry I think I probably messed something up when I tried to update a merge conflict. Can you update this PR?

@headius headius mentioned this pull request Jul 13, 2020
Merged
@headius
Copy link
Member

headius commented Jul 13, 2020

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 headius closed this Jul 13, 2020
@headius headius added this to the Invalid or Duplicate milestone Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kares kares

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Invalid or Duplicate
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@anukin @enebo @kares @headius