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.

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

SCLang: Too many args passed to certain 'inlined' methods #6302

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

JordanHendersonMusic
Copy link
Contributor

@JordanHendersonMusic JordanHendersonMusic commented May 9, 2024

Purpose and Motivation

When the compile/parser see a method with only a single call, or a call to super.something, it 'inlines' it.

For these methods, all the user's arguments were being passed through, regardless of how many were declared in the function signature.

This commit checks this, and removes any excess args from the stack.

Fixes #1087

Here is some code to demonstrate the error, before this commit, b and c would be set.

TestCopyArgs {
    var a, b, c;
    *new { |a| ^super.newCopyArgs(a) }
}
TestCopyArgs(\a, \b, \c).dump

Types of changes

  • Bug fix

This should not break any code, and if it does, that code is wrong.

To-do list

  • Code is tested
  • All tests are passing
  • Appeasing the linter
  • This PR is ready for review

@JordanHendersonMusic JordanHendersonMusic added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label May 9, 2024
@JordanHendersonMusic JordanHendersonMusic force-pushed the topic/fix-too-many-args-pushed branch 2 times, most recently from ffe7f32 to a89bea0 Compare May 9, 2024 18:40
@JordanHendersonMusic

This comment was marked as resolved.

Copy link
Member

@telephon telephon left a comment

Choose a reason for hiding this comment

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

Perhaps you could add a test that checks that correct ellipsis argument passing (still) works, too:

TestCorrectArgCountObj {
	var <a, <b, <c;
	*new {|...a| ^super.newCopyArgs(a) }
}

should make a == [\a, \b, \c] and b = c = nil.

And

TestCorrectArgCountObj {
	var <a, <b, <c;
	*new {|...a| ^super.newCopyArgs(*a) }
}

should make a = \a, b = \b, c = \c as expected.

When the compile/parser see a method with only a single call, or a call to `super.something`, it 'inlines' it.

For these methods, all the users arguments were being passed through, regardless of how many were declared in the function signature.

This commit checks this, and removes any excess args from the stack.

Closes supercollider#1087
@JordanHendersonMusic
Copy link
Contributor Author

@telephon all done.

@capital-G
Copy link
Contributor

Bump @telephon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"
Projects
Status: Waiting for review
Development

Successfully merging this pull request may close these issues.

newCopyArgs copies too many args
3 participants