atilaneves / automem

C++-style automatic memory management smart pointers for D
BSD 3-Clause "New" or "Revised" License
86 stars 15 forks source link

Vector.opIndex violates return #64

Open dkorpel opened 3 years ago

dkorpel commented 3 years ago

https://github.com/atilaneves/automem/blob/8f61747f437b7624ad4b44085e9525fc143a9e6c/source/automem/vector.d#L208-L213

struct Vector(E, Allocator = typeof(theAllocator)) if(isAllocator!Allocator) {
...
    E[] _elements;
...
    /// Access the ith element. Can throw RangeError.
    ref inout(E) opIndex(long i) scope return inout {
        if(i < 0 || i >= length)
            mixin(throwBoundsException);
        return _elements[i.toSizeT];
    }
...

The signature is incorrect. Because this is passed by ref and the function returns by ref, the function is return-ref so it may return the address of this (this._elements), but not the value (this._elements[i]).

Blocking https://github.com/dlang/dmd/pull/12665#issuecomment-858836483

atilaneves commented 3 years ago

I'm confused. When does scope ever apply to a member function then?

dkorpel commented 3 years ago

I'm confused. When does scope ever apply to a member function then?

In this case it applies, and it means that the value this._elements cannot be escaped. Without it, you could assign this._elements to a global E[]. The return keyword could imply that this._elements can be returned, but the spec currently says that in the case of a ref return and a ref parameter (this is passed by ref), the return applies to a pointer to the parameter (&this) and not the parameter's values (&this._elements[0]).

dkorpel commented 3 years ago

I've made a forum post about it: https://forum.dlang.org/post/nbbtdbgifaurxoknyeuu@forum.dlang.org

atilaneves commented 3 years ago

I've made a forum post about it: https://forum.dlang.org/post/nbbtdbgifaurxoknyeuu@forum.dlang.org

I have a headache now... but thanks for taking the time to write the post and in so much detail.

It seems you're arguing in the post that it's currently impossible to write opIndex correctly? That is, how should this issue should be resolved in the context of automem?

dkorpel commented 3 years ago

That is, how should this issue should be resolved in the context of automem?

You have to remove return scope from opIndex so the function cannot be called on a scope Vector anymore (a breaking change for automem users). However, as I mention in a reply, I think we can simplify dip1000 such that return applies to both the ref and value and remove the whole split between ref return and value return. In that case the language changes and automem stays the same. I think it's best if Walter, who specified the current design, takes a look to see if there are any flaws in my reasoning. I think it will be a huge win for dip1000 if the ambiguity of the return storage class can be removed.