supercollider
SCLang: add keywords to newCopyArgs as constructors are messy with many instance variables
Purpose and Motivation
If you have a class with many instance variables and want to only initialise some of them, you currently need to call setters, which gets messy and can be overloaded.
Now you can call them like so...
super.newCopyArgs(1, 2, 3, foo: 10, bar: 20)
You can also directly expose the constructor like this...
*new { |...args, kwargs|
^super.performArgs(\newCopyArgs, args, kwargs)
}
In future this could become the default implementation of Object*new.
This PR applied this to SynthDef as an example.
Types of changes
- New feature
To-do list
- [x] Code is tested
- [ ] All tests are passing
- [ ] Updated documentation
- [x] This PR is ready for review
Interesting feature! I'm not starting a full review, just some quick comments after a glance at the updated documentation.
@mtmccrea thanks for the review, I've fixed the typos now.
I also discovered a bug, I think it was a long standing bug that was only triggered by the recent keyword argument improvements.
A {
var a, b;
*new { ^super.newCopyArgs(10, b: 1) }
}
The above method would not set b to 1 as the keyword arguments where not being passed to the underlying primitive. This was a compiler issue in call node and I just removed the optimisation when there are keywords. There might be a better fix, but because the byte-codes are complicated, numerous, and poorly documented in the code, I can't find it. This wasn't found before because there were very few places where primitives are called with keyword arguments, and even fewer where the method's type is changed from methNormal to methPrimitive.
Because this is an optimisation issue applied at the parsing/compiling stage, this bug could also have been avoided by writing any of the following...
*new {
\hello;
^super.newCopyArgs(10, b: 1)
}
*new { ^super.newCopyArgs(10, b: 1).init }
One benefit to this is that we don't need to create public getters and setters to initialise only some of the member variables in a constructor.
@mtmccrea did you have any more thoughts about this one? IMO, I think it simplifies constructors quite a lot and helps keep encapsulation as you don't need to access setters in the *new method, so I can't see any downside.
I agree this is a good addition. There's a meta discussion that could be had about proper/encouraged construction and setting of members, but that's on the class authoring side. The feature only adds more flexibility, and fixes a bug, so I'm in favor of it.
Unfortunately I'm not able to properly review the cpp code, so it would be good if someone more familiar with this could have a look.
Thanks for your work on this!
@telephon I actually already had this hidden in there. I've made it more explicit now.
TLDR:
It throws a warning.
WARNING: Could not find instance variable with name 'y' in class 'TestConstructorA'
However, if the instance exists in a derived class the warning isn't shown.
TestConstructorBase {
var a, b, c;
// see 'd' here is set.
*newNonExistentKeywords { ^super.newCopyArgs(1, 2, 3, y: 10, d: 20, x: 30) }
value { ^[a, b, c] }
}
TestConstructorDerived : TestConstructorBase {
var d, e, f;
value { ^[a, b, c, d, e, f] }
}
TestConstructorBase.newNonExistentKeywords().() == [1, 2, 3]; // shows warning for y, d and x.
TestConstructorDerived.newNonExistentKeywords().() == [1, 2, 3, 20, nil, nil]; // d was set, shows warning for y and x.
This could be a little surprising in some circumstances, but I don't think it will cause any confusion?
This could be a little surprising in some circumstances, but I don't think it will cause any confusion?
So this means there are cases in which you call *newCopyArgs with keywords that do not exist, there won't be a warning, but it will just silently (correctly) fail to set them?
So this means there are cases in which you call *newCopyArgs with keywords that do not exist, there won't be a warning, but it will just silently (correctly) fail to set them?
If they exist on a child class, it will set them.
newCopyArgs always did this when dealing with positional arguments -> instance index, it just becomes more obvious when dealing with keyword arguments -> instance names.
Unchanged behaviour.
A {
var <>a, <>b;
*new { ^super.newCopyArgs(1, 2, 3, 4) }
}
B : A {
var <>c, <>d;
}
Object
a = A()
a.a // 1
a.b // 2
a.c // ERROR: Message 'c' not understood.
a.d // ERROR: Message 'd' not understood.
b = B()
b.a // 1
b.b // 2
b.c // 3
b.d // 4
Whether that is correct or not I am unsure...
Yes, that is unchanged – thanks fo rthe clear example, I think this should go somewhere in the help files. What I meant was the Could not find instance variable warning: is it omitted in the case of subclasses, and may that be a problem?
Sorry I don't really understand what you are asking. The warning triggers if an instance variable of that name cannot be found in the class being constructed.
Sorry! Yes, we want this warning. If this warning is always there when we want it to be there, all is well.
This seems like it is good to merge to me, any one else have any concerns?
The main benefit of this is to simply construction of objects when you want to initial members in an arbitrary order without exposing their public setter.
Just want to leave a note - if you experience a crash upon sclang startup referring to an unsuccessful integer assertion in
https://github.com/supercollider/supercollider/blob/a28099a199d22eea4fb63d792d83c2c86f97d690/lang/LangSource/PyrSlot64.h#L258-L261
This is caused by using a sclang build after this merge but using an older class library or vice versa. Took me some minutes to figure this out.
I don't think this is a problem of this PR, just if someone comes across it :)
