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

*/