Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vector.opIndex violates return #64

Open
dkorpel opened this issue Jun 15, 2021 · 5 comments
Open

Vector.opIndex violates return #64

dkorpel opened this issue Jun 15, 2021 · 5 comments

Comments

@dkorpel
Copy link
Contributor

dkorpel commented Jun 15, 2021

/// 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];
}

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 dlang/dmd#12665 (comment)

@atilaneves
Copy link
Owner

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

@dkorpel
Copy link
Contributor Author

dkorpel commented Jun 17, 2021

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
Copy link
Contributor Author

dkorpel commented Jun 18, 2021

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

@atilaneves
Copy link
Owner

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
Copy link
Contributor Author

dkorpel commented Jun 21, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants