Skip to content

[4688] add context and self to SizeFn#4714

Closed
anukin wants to merge 3 commits into
jruby:masterfrom
anukin:master
Closed

[4688] add context and self to SizeFn#4714
anukin wants to merge 3 commits into
jruby:masterfrom
anukin:master

Conversation

@anukin

@anukin anukin commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

@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
Comment thread core/src/main/java/org/jruby/RubyArray.java Outdated
return new SizeFn() {
@Override
public IRubyObject size(IRubyObject[] args) {
public IRubyObject size(IRubyObject[] args, ThreadContext context, IRubyObject self1) {

@kares kares Jul 12, 2017

Copy link
Copy Markdown
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)

@anukin anukin Jul 13, 2017

Copy link
Copy Markdown
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
Comment thread core/src/main/java/org/jruby/RubyArray.java Outdated
return new SizeFn() {
@Override
public IRubyObject size(IRubyObject[] args) {
public IRubyObject size(IRubyObject[] args, ThreadContext context, IRubyObject self1) {

@kares kares Jul 12, 2017

Copy link
Copy Markdown
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 ...

@anukin anukin Jul 13, 2017

Copy link
Copy Markdown
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
Comment thread 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.*;

@kares kares Jul 12, 2017

Copy link
Copy Markdown
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 😞

@anukin anukin Jul 13, 2017

Copy link
Copy Markdown
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

anukin commented Jul 13, 2017

Copy link
Copy Markdown
Contributor Author

@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

anukin commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

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

kares
kares reviewed Jul 18, 2017
View reviewed changes
Comment thread 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);

@kares kares Jul 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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)

@anukin anukin Jul 18, 2017

Copy link
Copy Markdown
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

anukin commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

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

@enebo

enebo commented Jul 18, 2017

Copy link
Copy Markdown
Member

@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
Comment thread 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);

@kares kares Jul 18, 2017

Copy link
Copy Markdown
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

anukin commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

@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

kares commented Jul 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

enebo commented Jan 29, 2018

Copy link
Copy Markdown
Member

@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

headius commented Jul 13, 2020

Copy link
Copy Markdown
Member

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 kares approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Invalid or Duplicate

Development

Successfully merging this pull request may close these issues.

4 participants

@anukin @enebo @kares @headius