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

opApply example isn't compliant with spec #372

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Inkrementator
Copy link

c9bf6f2 The spec states:

If the result is nonzero, apply must cease iterating and return that value.

The other commit add some other small improvements to make the example copy-pastable with minimal friction.

@Inkrementator
Copy link
Author

The spec also says that the signature should be
int opApply(scope int delegate(ref Tree) dg);
I think ref makes little sense, since actually using that would break loop invariance.
Should we add scope? Semantically, it makes sense, but it might confuse newcomers

@schveiguy
Copy link

The scope has a specific purpose. It means that the delegate will not be stored somewhere when the opApply returns. This allows the compiler to avoid allocating a closure.

In fact, it is crucial to do this for opApply, because all foreach bodies where opApply is used are rewritten as functions. Without scope, every foreach statement becomes a GC allocation.

Why ref? D allows you to specify non-ref or ref for the parameter, but just use one opApply that accepts by ref. This goes all the way back to the D1 days. So using ref just allows more functionality.

gems/opdispatch-opapply.md Outdated Show resolved Hide resolved
gems/opdispatch-opapply.md Outdated Show resolved Hide resolved
gems/opdispatch-opapply.md Outdated Show resolved Hide resolved
@ntrel
Copy link
Contributor

ntrel commented Oct 6, 2023

D allows you to specify non-ref or ref for the parameter, but just use one opApply that accepts by ref.

I've made a spec update: dlang/dlang.org#3699

@Inkrementator
Copy link
Author

Without scope, every foreach statement becomes a GC allocation.

Good to know. Am I right to assume that this is (mostly) only important for libraries? I always assumed the compiler or at least the optimizer could infer stuff like this, and annotating it is mostly for documentation and making sure that safe code doesn't rely on unsupported safeness of the code, leading to future breakage.

@schveiguy
Copy link

The optimizer can easily inline an opApply call, and therefore the lambda. Most likely this happens regardless of whether the delegate is marked scope or not.

But this is all after the frontend has decided whether the function is @nogc or not.

@schveiguy
Copy link

Aaaand, dmd just lets it happen. I guess this is a really old bug, which I actually commented on...

https://issues.dlang.org/show_bug.cgi?id=16193

But I don't think it's fixed, I just tried this and got garbage.

@schveiguy
Copy link

I would still recommend using scope, because at some point this could get "fixed".

@Inkrementator
Copy link
Author

Force pushed because I accidentally merged instead of rebased upstream

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

Successfully merging this pull request may close these issues.

3 participants