bmx-ng / bcc

A next-generation bcc parser for BlitzMax
zlib License
33 stars 12 forks source link

Array of Struct index data is copied instead of referenced #557

Open davecamp opened 3 years ago

davecamp commented 3 years ago

Win x64 v129.3.45 release

SuperStrict

Struct Test
    Field x:Float
    Field y:Float
    Field z:Float

    Method Set(x_:Float, y_:Float, z_:Float)
        x = x_
        y = y_
        z = z_
    EndMethod
EndStruct

Local data:Test[5]
For Local i:Int = 0 Until 5
    data[i].Set(i, i, i)
Next

I was expecting this code to alter the Struct instance values at the index of the array. However the output code works on a temporary copy of the struct data from the index, and then the copy goes out of scope without affecting the contents of the array.

c output for the 'For loop' creates a copy to do the work with, which gets discarded without affecting the array contents.

BBINT bbt_i=0;
for(;(bbt_i<5);bbt_i=(bbt_i+1)){
    struct _m_untitled1_Test bbt_=((struct _m_untitled1_Test*)BBARRAYDATA(bbt_data,1))[((BBUINT)bbt_i)];
    __m_untitled1_Test_Set_v_fff((struct _m_untitled1_Test*)&bbt_,((BBFLOAT)bbt_i),((BBFLOAT)bbt_i),((BBFLOAT)bbt_i));
}
davecamp commented 3 years ago

It looks like I can get around this by using

For Local i:Int = 0 Until 5
    Local temp:Test Ptr = Varptr data[i]
    temp.set(i, i, i)
Next

But the debugger doesn't like it so much, even though it seems to produce the correct values

For Local i:Int = 0 Until 5
    Local temp:Test Ptr = Varptr data[i]
    temp.set(i, i, i)
    Print data[i].x + ", " + data[i].y + " " + data[i].z
Next

outputs:

0.000000000, 0.000000000 0.000000000
1.00000000, 1.00000000 1.00000000
2.00000000, 2.00000000 2.00000000
3.00000000, 3.00000000 3.00000000
4.00000000, 4.00000000 4.00000000

with the debugger showing this after the loop has finished:

image

davecamp commented 3 years ago

Looks like the debugger is stepping through the array by 'size of Float' as opposed to 'size of Struct Test'?

HurryStarfish commented 3 years ago

I was expecting this code to alter the Struct instance values at the index of the array. However the output code works on a temporary copy of the struct data from the index, and then the copy goes out of scope without affecting the contents of the array.

I'm not sure I would consider this a bug. It is more of a direct consequence of the nature of Structs being value types, which means that they're copied on assignment - this is why one needs to be careful with mutable structs (and should avoid them unless necessary). I think it would be possible to change this behaviour for arrays, to make your example work. But that would only work for arrays, which would then behave differently from any custom collection type with an Operator []. I am not sure if that would be a good idea... it may actually end up making things more confusing.

Local temp:Test Ptr = Varptr data[i] temp.set(i, i, i)

This however certainly looks like a bug to me. Your Struct Test has a Set method, but pointers do not. temp[0].Set(i, i, i) should compile... temp.Set(i, i, i) shouldn't. Possibly related to #553?

davecamp commented 3 years ago

I'm not sure I would consider this a bug. It is more of a direct consequence of the nature of Structs being value types, which means that they're copied on assignment.

For sure it's a bug. The source code is not making an assignment. It is accessing an existing element in an array. There is no assignment happening in the original source, [edit] other than assigning Field values to the existing instance in the array.

I do agree that the pointer code should not work as is though, and of course the debugger output is wrong.

HurryStarfish commented 3 years ago

There is no assignment happening in the original source

No, but there's array indexing to retrieve an element (data[i]). You are expecting that operation to act like dereferencing a pointer and to give you a reference to the original element in the array (which, as I said, I'm sure can be done). What I'm trying to say is that personally, I think it also makes sense for it to act like retrieving an element from a list, a map, or any other custom collection with a Method Operator [] would... which is the current behaviour.

davecamp commented 3 years ago

I hear what you're saying, however with our personal opinions aside, we should go by the tradition (and documentation) of BlitzMax syntax. Ignoring custom type operator[] access for the moment, BlitzMax has always had 2 distinct and different syntax for copying an array element (via slicing) vs referencing an array element (with the element access [ ] syntax).

If using an element access syntax is going to copy the Struct data as opposed to reference it then the language syntax for arrays has become/is becoming ambiguous.

Also, if Struct is going to be treated very differently to Type then the documentation needs updating to reflect this, including the examples that frequently reference Type as a structure.

GWRon commented 2 years ago

the fix looks ... quite "cleaning up". Nice job.

GWRon commented 1 year ago
SuperStrict
Framework brl.StandardIO

Struct STest
    Field X:Int
    Field Y:Int

    Method SetX(value:Int)
        X = value
    End Method
End Struct

Local myArr:STest[5]
myArr[0].SetX(10)
Print myArr[0].X

myArr[0].Y = 10
Print myArr[0].Y

Local mySingle:STest
mySingle.SetX(20)
Print mySingle.X

Output:

0
10
20

So the "SetX()" does not work (as Col wrote, it actually does things to a copy of the struct)..., while direct assignment works.

I would say this is a bug and not intented.

Edit: tested with current BCC (compiled 5 minutes ago - from current sources)