Comment #0 by ellery-newcomer — 2012-07-25T14:06:24Z
code:
mixin template T(string i) {
auto j(string s="a", U)(U u1, U u2) {
return 0;
}
auto j(int i,string s="a", W)(W u1, W u2) {
return i;
}
mixin("
class F" ~ i ~ " {
auto j(string s=\"a\", U)(U u1, U u2) {
return this.outer.t" ~ i ~ ".j!(s,U)(u1,u2);
}
auto j(int i,string s=\"a\", W)(W u1, W u2) {
return this.outer.t" ~ i ~ ".j!(i,s,W)(u1,u2); <- dmd is giving error for j!(...).j's type
}
}
auto f"~i~"() {
return new F"~i~"();
}
");
}
class X {
mixin T!("1") t0;
alias t0 t1;
}
void main (){
X x = new X();
x.f1().j!(3,"a")(2.2, 3.3);
}
fireworks:
tok.d(15): Error: function expected before (), not 'this.this.j!(3,"a",double)'
tok.d(31): Error: template instance tok.X.T!("1").F1.j!(3,"a",double) error instantiating
Comment #1 by jfanatiker — 2012-11-20T07:31:13Z
Can also reproduced with:
https://github.com/eskimor/phobos/blob/new_signal/std/signals.d
And is currently a blocker for a full signals2 implementation.
Without it, I can only provide the FullSignal struct, which is good but does not support easily restriction of access to emit(). So users would have to redundantly create boilerplate code. Another option would be to use string mixins, but the syntax would not be as nice, so I would like to stick with the current implementation.
To summarize the problem: A mixin seems not to work with methods that are templates themselves, resulting in strange errors like:
std/signals.d(730): Error: no overload matches for disconnect(string method,ClassType)
or
std/signals.d(633): Error: function expected before (), not 'a.connect!("watch")'
Tested with dmd 2.060 and current master (7c370f71641c4408ddc9ebd5709e6e182e34ad2b).
Comment #2 by monarchdodra — 2013-01-18T06:07:40Z
It would appear the problem lies between a combination of mixin identifier, and template overload:
Here is a reduced test case:
//----
mixin template T() {
void k()(){}
void j()(){}
void j(int i)(){}
}
class X
{
mixin T t0;
}
void main (){
X x;
x.k!()(); //Fine
x.j!()(); //Fine
x.t0.k!()(); //Fine
x.t0.j!()(); //Derp
}
//----
main.d(12): Error: function expected before (), not 'x.j!()'
//-----
(In reply to comment #1)
> Can also reproduced with:
> https://github.com/eskimor/phobos/blob/new_signal/std/signals.d
>
> And is currently a blocker for a full signals2 implementation.
I don't know how you are affected by this, but you can workaround the problem by avoiding the template overload ambiguity. For example, this seems to work:
//----
mixin template T(string i) {
private {
auto j1(string s="a", U)(U u1, U u2) {
return j!(s, U)(u1, u2);
}
auto j2(int i,string s="a", W)(W u1, W u2) {
return j!(i, s, W)(u1, u2);
}
}
auto j(string s="a", U)(U u1, U u2) {
return 0;
}
auto j(int i,string s="a", W)(W u1, W u2) {
return i;
}
mixin("
class F" ~ i ~ " {
auto j(string s=\"a\", U)(U u1, U u2) {
return this.outer.t" ~ i ~ ".j1!(s,U)(u1,u2);
}
auto j(int i,string s=\"a\", W)(W u1, W u2) {
return this.outer.t" ~ i ~ ".j2!(i,s,W)(u1,u2);
}
}
auto f"~i~"() {
return new F"~i~"();
}
");
}
class X {
mixin T!("1") t0;
alias t0 t1;
}
void main (){
X x = new X();
x.f1().j!(3,"a")(2.2, 3.3);
}
//----
Comment #3 by jfanatiker — 2013-01-18T07:04:59Z
Ah, I see: So the issue is that template overloads don't work. Thanks for investigating this!
Unfortunately it does not really help for std.signals2, because renaming the methods to connectDirectly, connectViaDelegate, .... seems rather ugly, although I don't think it is a good idea to alter a public API because of workarounds. I would be fine, if it only affected private parts.
Comment #4 by monarchdodra — 2013-01-18T08:15:53Z
(In reply to comment #3)
> Ah, I see: So the issue is that template overloads don't work.
In the context of template mixins with mixinIdentifiers only. Template overloads, as a general rule, do work.
> Thanks for
> investigating this!
>
> Unfortunately it does not really help for std.signals2, because renaming the
> methods to connectDirectly, connectViaDelegate, .... seems rather ugly,
> although I don't think it is a good idea to alter a public API because of
> workarounds. I would be fine, if it only affected private parts.
Technically, I didn't rename anything. I merelly added some extra functions to remove the ambiguity in the context of the mixin identifier.
Yes, it is ugly, but the public interface is intact.
Comment #5 by jfanatiker — 2013-01-18T09:17:10Z
> In the context of template mixins with mixinIdentifiers only. Template
> overloads, as a general rule, do work.
Yeah, I meant that. I used to think that template functions in a mixin
template don't work, but it is just overloads. Thanks for pointing this
out.
> > Unfortunately it does not really help for std.signals2, because
> renaming the
> > methods to connectDirectly, connectViaDelegate, .... seems rather
> ugly,
> > although I don't think it is a good idea to alter a public API
> because of
> > workarounds. I would be fine, if it only affected private parts.
>
> Technically, I didn't rename anything. I merelly added some extra
> functions to
> remove the ambiguity in the context of the mixin identifier.
>
> Yes, it is ugly, but the public interface is intact.
But the instantiation of a signal and the access:
mixin T!("1") t0;
alias t0 t1;
x.f1().j!(3,"a")(2.2, 3.3);
vs
mixin T!() t0;
x.t0.j!(3,"a")(2.2, 3.3);
would change, which is part of the public accessible API. If the
workaround gets removed, people would have to adopt their code, which is
kind of unacceptable for phobos.
Best regards,
Robert
Comment #6 by monarchdodra — 2013-01-18T09:49:28Z
(In reply to comment #5)
>
> But the instantiation of a signal and the access:
> mixin T!("1") t0;
> alias t0 t1;
> x.f1().j!(3,"a")(2.2, 3.3);
>
> vs
> mixin T!() t0;
> x.t0.j!(3,"a")(2.2, 3.3);
>
> would change, which is part of the public accessible API. If the
> workaround gets removed, people would have to adopt their code, which is
> kind of unacceptable for phobos.
>
> Best regards,
>
> Robert
I don't understand. You used to have:
mixin T!("1") t0;
alias t0 t1;
x.f1().j!(3,"a")(2.2, 3.3);
And I proposed something that worked with
mixin T!("1") t0;
alias t0 t1;
x.f1().j!(3,"a")(2.2, 3.3);
Nothing changed. Where did:
mixin T!() t0;
x.t0.j!(3,"a")(2.2, 3.3);
Come from?
All you should need is a private "man in the middle"...
Well, I don't have your end code, so I don't know how acceptable that is anyways. Good luck :)
Comment #7 by jfanatiker — 2013-01-18T10:39:34Z
Ha! Sorry :-) I completely forgot about the example of Ellery Newcomer (And I wondered already how you made up such an elaborate solution, from such a simple problem ;-) ).
The code I am concerned with is:
https://github.com/eskimor/phobos/blob/new_signal/std/signals.d
The template mixin code in particular:
mixin template Signal(Args...)
{
private final void emit( Args args )
{
full.emit(args);
}
final void connect(string method, ClassType)(ClassType obj) if(is(ClassType == class) && __traits(compiles, {void delegate(Args) dg=mixin("&obj."~method);}))
{
full.connect!method(obj);
}
final void connect(ClassType)(ClassType obj, void delegate(ClassType obj, Args) dg) if(is(ClassType == class))
{
full.connect(obj, dg);
}
final void strongConnect(void delegate(Args) dg)
{
full.strongConnect(dg);
}
final void disconnect(string method, ClassType)(ClassType obj) if(is(ClassType == class) && __traits(compiles, {void delegate(Args) dg=mixin("&obj."~method);}))
{
full.disconnect!method(obj);
}
final void disconnect(ClassType)(ClassType obj, void delegate(ClassType, T1) dg) if(is(ClassType == class))
{
full.disconnect(obj, dg);
}
final void disconnect(ClassType)(ClassType obj) if(is(ClassType == class))
{
full.disconnect(obj);
}
final void strongDisconnect(void delegate(Args) dg)
{
full.strongDisconnect(dg);
}
final ref RestrictedSignal!(Args) restricted() @property
{
return full.restricted;
}
private FullSignal!(Args) full;
}
Use case:
class Button {
mixin Signal!() clicked;
void click() {
clicked.emit();
}
}
import std.stdio;
void main() {
Button b=new Button;
b.clicked.strongConnect(() {writeln("I was clicked!");});
b.click();
}
(In reply to comment #8)
> I don't test std.signal2 module, but the two test cases work with my patch.
>
> https://github.com/D-Programming-Language/dmd/pull/1660
Awesome! This is really great news! Thank you alot! :-) :-) :-) :-D :-) !!!
I tried it and the problem seems to be fixed, but I ran into different issues (forward reference errors and runtime errors). I still have to figure out whether this is related to your changes or not. (The runtime errors are probably because I am still linking to an old phobos version, as I am not able to build the current one, due to oom problems.)
Comment #11 by andrej.mitrovich — 2013-07-09T10:51:10Z
(In reply to comment #10)
> (In reply to comment #8)
> > I don't test std.signal2 module, but the two test cases work with my patch.
> >
> > https://github.com/D-Programming-Language/dmd/pull/1660
>
> std.signal2 works too! Thank you!
@[email protected]: I've tried your std.signals by compiling with 'dmd -c -o- -unittest signals.d' (the unittest switch is important here), specifically I've tried two commits for signals2:
04c951e34623e9365a3874c89f43eb997a7b376c (dav1d told me you said this might work)
4f7aaba0135bdfebfe54cbd645ca3652b0b0eb7a (git-head)
I've tried them both with the current state of Pull 1660, but compilation fails due to those mixin conflicts. Can you verify this?
Comment #12 by jfanatiker — 2013-07-09T11:57:15Z
> I've tried them both with the current state of Pull 1660, but compilation fails
> due to those mixin conflicts. Can you verify this?
HEAD definitely won't work at all. The older commit used to work for me, but pull 1660 changed quite significantly since then. Seeing that there is real use of my implementation I'm shifting it up in my priority list, prepare to see some commits in the next days. I will comment on pull 1660 if I find any issues.
Best regards,
Robert
Comment #13 by andrej.mitrovich — 2013-07-09T15:29:03Z
(In reply to comment #12)
> > I've tried them both with the current state of Pull 1660, but compilation fails
> > due to those mixin conflicts. Can you verify this?
>
> HEAD definitely won't work at all. The older commit used to work for me, but
> pull 1660 changed quite significantly since then. Seeing that there is real use
> of my implementation I'm shifting it up in my priority list, prepare to see
> some commits in the next days. I will comment on pull 1660 if I find any
> issues.
>
> Best regards,
> Robert
Btw why are you using mixin templates instead of a struct? I know the current std.signals uses it, but it's broken because people keep running into Issue 5028.
Comment #14 by andrej.mitrovich — 2013-07-09T15:29:34Z
> Btw why are you using mixin templates instead of a struct? I know the current
> std.signals uses it, but it's broken because people keep running into Issue
> 5028.
Instead is not quite correct, the mixin is just a wrapper around the FullSignal struct.
Reasoning: So only containing object can emit signals. (emit is made private)
It is just a convenience wrapper which implements a good default behaviour in just one line of code. (Make emit private) Otherwise you would have to instantiate a FullSignal as private and manually provide methods for accessing the RestrictedSignal for making connections. Not too much work, but a little annoying.
You can of course use the struct directly if you want.
Comment #16 by andrej.mitrovich — 2013-07-10T07:30:35Z
(In reply to comment #15)
> > Btw why are you using mixin templates instead of a struct? I know the current
> > std.signals uses it, but it's broken because people keep running into Issue
> > 5028.
>
> Instead is not quite correct, the mixin is just a wrapper around the FullSignal
> struct.
Ah you mean 'RestrictedSignal', the other one has a private alias. Ok then.
Comment #17 by jfanatiker — 2013-07-12T14:43:48Z
I just finished a new implementation, replacing the template mixin with a string mixin. You can find it here:
https://github.com/phobos-x/phobosx/blob/master/source/phobosx/signal.d
All unittests pass, you don't need any patched compiler. I still have to add some more checks and do some polishing, I will also put it in the dub registry. But you and David seem to have an urgent need, so feel free to try it out immediately - Be my pre-alpha Testers :-)
Best regards,
Robert
Comment #18 by github-bugzilla — 2013-07-16T18:35:55Z