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

Specify opApply as an alias to a function template instance behavior #3859

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

Conversation

Bolpat
Copy link
Contributor

@Bolpat Bolpat commented Jun 26, 2024

In particular, cf. Issue 24633.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 26, 2024

Thanks for your pull request and interest in making D better, @Bolpat! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
17953 enhancement inout-like mechanism to infer function attributes from callback attributes
23666 enhancement Recognize template opApply pattern
24633 enhancement Document opApply as an alias to a function template instance

@Bolpat Bolpat changed the title Specify opApply as an alias to a function template instance Specify opApply as an alias to a function template instance behavior Jun 26, 2024
@ntrel
Copy link
Contributor

ntrel commented Jun 27, 2024

DAutoTest:

Searching for tabs
./spec/statement.dd:732:		S s;
make: *** [posix.mak:822: test] Error 1

spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
spec/statement.dd Outdated Show resolved Hide resolved
@ntrel
Copy link
Contributor

ntrel commented Jun 27, 2024

I think it would be better to implement 23666 than have to document the alias workaround. However, the latter does work.

@ntrel
Copy link
Contributor

ntrel commented Jun 27, 2024

I don't think this fixes 23116 - it's just a workaround for it.

@Bolpat
Copy link
Contributor Author

Bolpat commented Jun 27, 2024

I think it would be better to implement 23666 than have to document the alias workaround. However, the latter does work.

Maybe set the issue as wontfix then.

I don't think this fixes 23116 - it's just a workaround for it.

I agree, and I realized that almost immediately and removed from the commit message. In the dlang bot box, it’s not present anymore.

@Bolpat
Copy link
Contributor Author

Bolpat commented Jun 27, 2024

@ntrel, I now went over the relevant section with a text-to-speech tool and found (hopefully) the last issues. If the checks succeed, please have a look and merge if you don’t find any problems. Content-wise, I’m satisfied with how it looks and reads.

Comment on lines -709 to -715
$(GRAMMAR
$(GNAME OpApplyDeclaration):
`int opApply` `(` `scope` `int delegate` `(` $(I OpApplyParameters) `)` `dg` `)` `;`

$(GNAME OpApplyParameters):
*OpApplyParameter*
*OpApplyParameter*, *OpApplyParameters*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to just remove OpApplyParameter, these two are still useful as informative grammar IMO.

Copy link
Contributor Author

@Bolpat Bolpat Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not a real grammar entity and just hangs there. Formally speaking, a grammar has a root (Module for D) from which parsing starts. Every other non-terminal symbol is only useful when it’s referenced as the right-hand side of some other rule (and even that is not sufficient). There is no rule with OpApplyDeclaration and therefore, it’s useless. As far as the D grammar is concerned, opApply isn’t special and just like any other function declaration.

In principle, we could add an INFORMATIVE_GRAMMAR block (cf. #3860) such as:

int opApply ( scope int delegate Parameters body )

Honestly, I see little value in it. The examples do a great job at demonstrating how it’s done.

Apart from that, it’s technically wrong. The delegate type could given in various ways, such as DG which is an alias to a delegate type, or opApply could be a template that takes DG as a parameter. Or opApply can be an alias itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's informative as to what the symbol should resolve to.

$(PANEL
To support a `ref` iteration variable, the delegate must take a `ref` parameter:
To support a `ref` iteration, the delegate must take `ref` parameters:
Copy link
Contributor

@ntrel ntrel Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original wording was better, because a delegate could take one ref parameter and one non-ref. It's also best not to mix singular and plural.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be rephrased. I’m not happy with it either. I don’t like must here as it’s not a real requirement (such as opApply must return a nonzero result of body), it’s a mere conditional: If you want XYZ, do ABC.

Suggested change
To support a `ref` iteration, the delegate must take `ref` parameters:
To support iteration by reference, let the delegate take the respective parameters as `ref` parameters:

What do you think?

Comment on lines -721 to -723
$(P where each $(I OpApplyParameter) of `dg` must match a $(GLINK ForeachType)
in a $(GLINK ForeachStatement),
otherwise the *ForeachStatement* will cause an error.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this paragraph too (though it would need to use ParameterDeclaration if you reinstate that change).


$(SPEC_RUNNABLE_EXAMPLE_RUN
--------------
class Foo
class Tree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original example was simpler and clearer. The new example shows why opApply is needed though.

spec/statement.dd Show resolved Hide resolved
spec/statement.dd Show resolved Hide resolved
spec/statement.dd Show resolved Hide resolved
@ntrel
Copy link
Contributor

ntrel commented Jun 28, 2024

please have a look and merge if you don’t find any problems

Just to mention, I don't have merge rights ;-)

spec/statement.dd Outdated Show resolved Hide resolved
Co-authored-by: Nick Treleaven <[email protected]>
@Bolpat
Copy link
Contributor Author

Bolpat commented Jul 2, 2024

In all honesty, this is already too mission-creeped. Either merge this, or undo the changes except the new section, and put the improvements in a new PR.

@thewilsonator, review?

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

Successfully merging this pull request may close these issues.

3 participants