A small quiz (it's a reprise of an older discussion). This code compiles with no errors, but do you see anything wrong here?


// -----------------
struct Foo {
    int x;
    alias this = x;
}

class Bar {
    Foo[] data;

    this() {
        data.length = 10;
    }

    Foo opIndex(uint i) {
        return data[i];
    }

    void opIndexUnary(string op)(uint i) if (op == "++") {
        data[i]++;
    }

    void opIndexUnaryRight(string op)(uint i) if (op == "++") {
        data[i]++;
    }
}

void main() {
    auto barfoo = new Bar;
    ++barfoo[3];
    assert(barfoo.data[3] == 1);
    barfoo[3]++;
    assert(barfoo.data[3] == 2);
}
// -----------------


Do you think this code should produce some compilation errors?

Bye,
bearophile

On 01/01/2013 09:52 AM, bearophile wrote:
> void opIndexUnaryRight(string op)(uint i) if (op == "++") {
> data[i]++;
> }

I admit that I could not see the error as I was reading the code. I thought that I was learning something new from that code. ;)

> Do you think this code should produce some compilation errors?

That is hard to answer. It makes me think that perhaps the special function names should all start with double underscores. Only then the compiler would have the right to produce a compilation error for the code above. :-/

Ali

On Tuesday, 1 January 2013 at 17:52:20 UTC, bearophile wrote:
> A small quiz (it's a reprise of an older discussion). This code compiles with no errors, but do you see anything wrong here?

 Well I see that you have opIndexUnary twice; According to the manual you wouldn't need as it would rewrite the code so you only need it once; Also they return void when they should return the value of x before/after the change.

 Also although the alias this makes the number increment, it should be specifying 'x', not that it seems to make a difference.

> Do you think this code should produce some compilation errors?

 As it is I don't see any problems...

On Tuesday, 1 January 2013 at 17:52:20 UTC, bearophile wrote:
> A small quiz (it's a reprise of an older discussion). This code compiles with no errors, but do you see anything wrong here?
>
>
> // -----------------
> struct Foo {
>     int x;
>     alias this = x;
> }
>
> class Bar {
>     Foo[] data;
>
>     this() {
>         data.length = 10;
>     }
>
>     Foo opIndex(uint i) {
>         return data[i];
>     }
>
>     void opIndexUnary(string op)(uint i) if (op == "++") {
>         data[i]++;
>     }
>
>     void opIndexUnaryRight(string op)(uint i) if (op == "++") {
>         data[i]++;
>     }
> }
>
> void main() {
>     auto barfoo = new Bar;
>     ++barfoo[3];
>     assert(barfoo.data[3] == 1);
>     barfoo[3]++;
>     assert(barfoo.data[3] == 2);
> }
> // -----------------
>
>
> Do you think this code should produce some compilation errors?
>
> Bye,
> bearophile

YES. Or should I say: There needs to be a way to diagnose these kinds of errors. I think there should be a "operator" keyword to protect yourself. I'd prefer it be mandatory, but even introducing it as optional would help:

//----
operator void opIndexUnaryRight(string op)(uint i);
//----
Error: No operator opIndexUnaryRight. Perhaps you meant opIndexUnary?
//----

This is especially improtant in that sometimes, the operator name is a guessing game...

==========================================
Related: I fixed a similar bug in std.container.DList recently: Can you spot the error?

//----
DList opOpassign(string op, Stuff)(Stuff rhs)  	
if (op == "~" && is(Stuff == DList))
//----

The fact that it made it into the standard library is, IMO, a tell tale sign of a big problem. A "smoking gun".

On Tuesday, 1 January 2013 at 21:19:04 UTC, monarch_dodra wrote:
> ==========================================
> Related: I fixed a similar bug in std.container.DList recently: Can you spot the error?
>
> //----
> DList opOpassign(string op, Stuff)(Stuff rhs)  	
> if (op == "~" && is(Stuff == DList))
> //----
>
> The fact that it made it into the standard library is, IMO, a tell tale sign of a big problem. A "smoking gun".

 I know it should be opOpAssign, but is easy to mess up...

On Tuesday, 1 January 2013 at 19:32:10 UTC, Era Scarecrow wrote:
> On Tuesday, 1 January 2013 at 17:52:20 UTC, bearophile wrote:
>> A small quiz (it's a reprise of an older discussion). This code compiles with no errors, but do you see anything wrong here?
>
>  Well I see that you have opIndexUnary twice; According to the manual you wouldn't need as it would rewrite the code so you only need it once; Also they return void when they should return the value of x before/after the change.
>
>  Also although the alias this makes the number increment, it should be specifying 'x', not that it seems to make a difference.

The notion of "right" does not really mean "on which side the operator is". Rather, "which operand does it apply to":

"A + B"

If A doesn't provide opBinary, then the compiler will move on to see if B provides opBinaryRight.

In the context of opUnary (and opIndexUnary), it makes no sense to talk about a "right" version. This should be caught (IMO) by the compiler.

Also, opUnary *can* return a void. However, in that case, the compiler will be unable to auto generate the post version of the unary operation. This is a legal design decision, taken (for example) by std.container.Array.

Era Scarecrow:

>  Well I see that you have opIndexUnary twice; According to the manual you wouldn't need as it would rewrite the code so you only need it once;

And as you have seen if you remove the useles opIndexRight the program keeps compiling with no errors and keeps asserting at run-time:



struct Foo {
    int x;
    alias this = x;
}

class Bar {
    Foo[] data;

    this() {
        data.length = 10;
    }

    Foo opIndex(uint i) {
        return data[i];
    }

    void opIndexUnary(string op)(uint i) if (op == "++") {
        data[i]++;
    }
}

void main() {
    auto barfoo = new Bar;
    ++barfoo[3];
    assert(barfoo.data[3] == 1);
    barfoo[3]++;
    assert(barfoo.data[3] == 2);
}


Bye,
bearophile

1/2/2013 7:52 AM, bearophile пишет:
> Era Scarecrow:
>
>>  Well I see that you have opIndexUnary twice; According to the manual
>> you wouldn't need as it would rewrite the code so you only need it once;
>
> And as you have seen if you remove the useles opIndexRight the program
> keeps compiling with no errors and keeps asserting at run-time:
>
>
>
> struct Foo {
>      int x;
>      alias this = x;

Implicit conversion to int...

> }
>
> class Bar {
>      Foo[] data;
>
>      this() {
>          data.length = 10;
>      }
>
>      Foo opIndex(uint i) {
>          return data[i];
>      }
>
>      void opIndexUnary(string op)(uint i) if (op == "++") {
>          data[i]++;

...and ++ somehow works with rvalue.

The fact that it's allowed is dangerous if you ask me.

>      }
> }
>
> void main() {
>      auto barfoo = new Bar;
>      ++barfoo[3];
>      assert(barfoo.data[3] == 1);
>      barfoo[3]++;
>      assert(barfoo.data[3] == 2);
> }
>
>
> Bye,
> bearophile


-- 
Dmitry Olshansky

On 01/02/2013 01:39 AM, Dmitry Olshansky wrote:
> 1/2/2013 7:52 AM, bearophile пишет:

>> struct Foo {
>> int x;
>> alias this = x;
>
> Implicit conversion to int...

> }

>> class Bar {
>>      Foo[] data;
>
>>      this() {
>>          data.length = 10;
>>      }
>
>>      Foo opIndex(uint i) {
>>          return data[i];
>>      }
>
>> void opIndexUnary(string op)(uint i) if (op == "++") {
>> data[i]++;
>
> ...and ++ somehow works with rvalue.

I am not sure that it is an rvalue in this case. I think x is used directly as an lvalue. Since the post-increment operator cannot be overloaded in D, the compiler writes the previous expression as the equivalent of the following:

{
    int oldValue = data[i];
    ++data[i];
    oldValue;
}

Since data[i].x is an lvalue int, there shouldn't be a problem.

The problem that I have seen here is that although the programmer thinks that the post-increment operation is being defined by opIndexUnaryRight(), opIndexUnaryRight() is not a special name. We have seen a similar issue recently with opGet(). These names happen to look like operator overloads but they are not.

Ali

On Wednesday, 2 January 2013 at 03:52:21 UTC, bearophile wrote:
> Era Scarecrow:
>
>> Well I see that you have opIndexUnary twice; According to the manual you wouldn't need as it would rewrite the code so you only need it once;
>
> And as you have seen if you remove the useles opIndexRight the program keeps compiling with no errors and keeps asserting at run-time:
>
>
>
> struct Foo {
>     int x;
>     alias this = x;
> }
>
> class Bar {
>     Foo[] data;
>
>     this() {
>         data.length = 10;
>     }
>
>     Foo opIndex(uint i) {
>         return data[i];
>     }
>
>     void opIndexUnary(string op)(uint i) if (op == "++") {
>         data[i]++;
>     }
> }
>
> void main() {
>     auto barfoo = new Bar;
>     ++barfoo[3];
>     assert(barfoo.data[3] == 1);
>     barfoo[3]++;
>     assert(barfoo.data[3] == 2);
> }
>
>
> Bye,
> bearophile

I am really just guessing, but it appears that whatever the post increment op gets rewritten as takes either a reference to or a copy of what is being incremented and it involves opIndex, not opIndexUnary as suggested by TDPL p. 378 and p. 369.

If opIndex is changed to return ref Foo, everything works. (I had change alias this = x to alias x this - 2.60 64 bit). Here is code that shows when opIndexUnary and opIndex is called and when the assertion fails for version(B) - with ref and version(A) without ref.

Note that TDPL recommends *not* having opIndex return a ref (p. 378). Can we have out cake and eat it too?


import std.stdio, std.conv;

// -----------------
struct Foo {
  int x;
  alias x this;
  string toString() {
    return to!string(x);
  }
}

class Bar {
  Foo[] data;

  this() {
    data.length = 10;
  }

  version( A ) {
    Foo opIndex(uint i) {
      writefln("opIndex data[i]: %s",data[i]);
      return data[i];
    }
  }

  version( B ) {
    ref Foo opIndex(uint i) {
      writefln("opIndex data[i]: %s",data[i]);
      return data[i];
    }
  }

  Foo opIndexUnary(string op)(uint i) if (op == "++") {
    ++data[i];
    writefln("opIndexUnary data[i]: %s",data[i]);
    return data[i];
  }

}

void main() {
  auto barfoo = new Bar;
  assert(++barfoo[3] == 1);
  writeln("after ++barfoo[3]");
  assert(barfoo[3]++ == 1);
  writeln("after barfoo[3]++");
  assert(barfoo[3] == 2);             // Line 47 - fails for version(A)
  writeln("after barfoo[3]");
}
// -----------------


/*

$> dmd opndx.d -version=B
$> ./opndx
opIndexUnary data[i]: 1
after ++barfoo[3]
opIndex data[i]: 1
after barfoo[3]++
opIndex data[i]: 2
after barfoo[3]


$> dmd opndx.d -version=A
$> ./opndx
opIndexUnary data[i]: 1
after ++barfoo[3]
opIndex data[i]: 1
after barfoo[3]++
opIndex data[i]: 1
core.exception.AssertError@opndx(47): Assertion failure

*/