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

[Truffle] splat position; Thread#backtrace #3191

Merged
merged 5 commits into from
Jul 30, 2015
Merged

[Truffle] splat position; Thread#backtrace #3191

merged 5 commits into from
Jul 30, 2015
+124 −76

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented Jul 27, 2015

No description provided.

@pitr-ch pitr-ch self-assigned this Jul 27, 2015
@pitr-ch pitr-ch added this to the truffle-dev milestone Jul 27, 2015
@eregon
Copy link
Member

eregon commented Jul 27, 2015

I would call *arg in an argument list a rest argument instead of splat.
Why do you want the caller to perform the computation of the index, could Arity do it if we just pass required, optional and whether there is a rest argument?

@pitr-ch
Copy link
Member Author

pitr-ch commented Jul 27, 2015

I think what you suggest is not enough because of post-splat arguments. The position can be smaller than required+optional. It could be pre, optional, splat?, post though.

@eregon
Copy link
Member

eregon commented Jul 27, 2015

How so? The frame will contain the arguments in order, so for instance at the position of an optional argument there will be either the given value or the default one.

@pitr-ch pitr-ch changed the title [Truffle] track splat postition in Arity [Truffle] splat position; Thread#backtrace Jul 27, 2015
@pitr-ch pitr-ch force-pushed the master branch 2 times, most recently from 6e134f9 to 280a602 Compare July 28, 2015 14:30
eregon
eregon reviewed Jul 29, 2015
View reviewed changes
truffle/src/main/java/org/jruby/truffle/nodes/arguments/CheckArityNode.java
//TODO (MS): Check merge

//TODO (MS): Check merge
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these unintentional changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

done, what's our policy on these changes, separate commits or don't? I feel these trailing spaces as malformed code.

Copy link
Member

Choose a reason for hiding this comment

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

ah sorry I misread the diff, I thought the diff added the extra spaces.
Removing neighboring style issues is fine.

@pitr-ch pitr-ch force-pushed the master branch 2 times, most recently from d788fdc to 97f7e3b Compare July 29, 2015 18:01
@eregon
Copy link
Member

eregon commented Jul 29, 2015

Looks good, bonus point if you can rebase on master :)

@pitr-ch pitr-ch force-pushed the master branch 2 times, most recently from f196e52 to 11aae78 Compare July 30, 2015 13:29
@pitr-ch pitr-ch merged commit d79f665 into jruby:master Jul 30, 2015
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pitr-ch pitr-ch

Labels
None yet
Projects
None yet
Milestone
Non-Release
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pitr-ch @eregon @enebo