On 09/02/13 18:32, Joseph Rushton Wakeling wrote:
> On 02/09/13 17:44, Artur Skawina wrote:
>> The nasty part of that is that the type of .funcptr, hence also of the '_jump' pointer, is bogus. So calling via '_jump' directly can succeed, but do the wrong thing. It's a language issue; one reasonable workaround would be to define a MemberPtr type, which disallows direct calls.
> 
> SafeDelegate, perhaps?  Might be a worthwhile library addition.

"Fixing" type system bugs by adding library helpers is not really a good
idea. '.funcptr' returning incorrect type (and 'typeof(T.method)' etc)
needs to be fixed properly. Until that happens, the alternatives are:
a) convention - don't directly access these wrongly-typed pointers;
b) cast to (void*) (or the correct type) and back; ugly and still un@safe;
c) wrap the pointer, this way it's at least a little bit harder to
   mistakenly use the bogus type.

The last alternative could look similar to:

   struct MemberFunc(MF, T, DG) {
      MF mf;
      auto ref opCall(A...)(ref T o, auto ref A a) {
         DG dg = void; dg.ptr = &o; dg.funcptr = mf; return dg(a);
      }
   }

   auto memberFunc(T, DG)(ref T, DG dg) {
      auto fp = dg.funcptr;
      MemberFunc!(typeof(fp), T, DG) r /*@unsafe: = void*/;
      r.mf = fp;
      return r;
   }

   struct S {
      int a;
      double f(int b) { return a+b; }
   }

   void main() {
      auto s = S(3);
      auto mp = memberFunc(s, &s.f);
      mp(s, 5);
      auto s2 = S(4);
      mp(s2, 5);
   }

artur

On 02/09/13 19:31, Artur Skawina wrote:
> "Fixing" type system bugs by adding library helpers is not really a good
> idea. '.funcptr' returning incorrect type (and 'typeof(T.method)' etc)
> needs to be fixed properly. Until that happens, the alternatives are:
> a) convention - don't directly access these wrongly-typed pointers;
> b) cast to (void*) (or the correct type) and back; ugly and still un@safe;
> c) wrap the pointer, this way it's at least a little bit harder to
>     mistakenly use the bogus type.

Do you want to take a quick look at the proposed patch to std.random and see if there's anything obviously risky about it?  No pressure, but would be nice to have your comments.
https://github.com/D-Programming-Language/phobos/pull/1533/files

On 09/02/13 20:21, Joseph Rushton Wakeling wrote:
> On 02/09/13 19:31, Artur Skawina wrote:
>> "Fixing" type system bugs by adding library helpers is not really a good
>> idea. '.funcptr' returning incorrect type (and 'typeof(T.method)' etc)
>> needs to be fixed properly. Until that happens, the alternatives are:
>> a) convention - don't directly access these wrongly-typed pointers;
>> b) cast to (void*) (or the correct type) and back; ugly and still un@safe;
>> c) wrap the pointer, this way it's at least a little bit harder to
>>     mistakenly use the bogus type.
> 
> Do you want to take a quick look at the proposed patch to std.random and see if there's anything obviously risky about it?  No pressure, but would be nice to have your comments.
> https://github.com/D-Programming-Language/phobos/pull/1533/files

I'm not qualified.
But it looks like it could work, just that I'd document the hack (the fact
that '_skip' is not a normal, working function pointer, but it's actually a
pointer to a method, is missing an argument and is incorrectly typed.
Otherwise someone reading that code later will a) miss this fact, then b)
notice what's going on, and c) wonder if it's really ok, which may not be
obvious when looking just at the code, and not the patch that introduced it.
Or even - a1) modify the code w/o realizing that calling `_skip` is not valid...
So it needs to be well documented. No idea however how phobos handles this;
I try to avoid D's std libs at all costs. (Dealing with the bugs is much
more expensive than just writing everything from scratch). So ignore me. :)


artur

Am 02.09.2013 15:39, schrieb Artur Skawina:
>
>      this(this) { jump.ptr = &this; }
>

Just a warning: This can still easily crash due to D's struct move semantics. Structs are moved around sometimes without any postblit constructor or destructor being called, so fixing self-references like this won't work in general.

On 09/03/13 12:00, Sönke Ludwig wrote:
> Am 02.09.2013 15:39, schrieb Artur Skawina:
>>
>>      this(this) { jump.ptr = &this; }
>>
> 
> Just a warning: This can still easily crash due to D's struct move semantics. Structs are moved around sometimes without any postblit constructor or destructor being called, so fixing self-references like this won't work in general.

Yes. Thanks for catching this.
I tend to ignore the broken/hacky/incomplete parts of the language,
and forgot about it.


artur

On 03/09/13 12:00, Sönke Ludwig wrote:
> Just a warning: This can still easily crash due to D's struct move semantics.
> Structs are moved around sometimes without any postblit constructor or
> destructor being called, so fixing self-references like this won't work in general.

Is the patch to Phobos risky, then?  I'll switch to an alternative design if so.

On 09/04/13 13:42, Joseph Rushton Wakeling wrote:
> On 03/09/13 12:00, Sönke Ludwig wrote:
>> Just a warning: This can still easily crash due to D's struct move semantics.
>> Structs are moved around sometimes without any postblit constructor or
>> destructor being called, so fixing self-references like this won't work in general.
> 
> Is the patch to Phobos risky, then?  I'll switch to an alternative design if so.

No, like i said, that's not how I'd do it (I would make `this` explicit and avoid all those issues), but that last patch /will/ work.

The problem is with *pointers* inside an object that point to that same object. 'Pointers' includes delegates (those contain a context pointer). You're not storing any pointers, so it's ok. It was my first quick "fix", which attempted to update the context pointer in the postblit, that was wrong.

artur