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

Fixes #3028. Add tests for mixin application constant constructor #3032

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

sgrekhov
Copy link
Contributor

No description provided.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

LGTM.

I was just wondering about one thing: The tests starting with ...A03_t06... seem to differ by having different kinds of parameters (optional named, required named). I couldn't see an obvious reason why this would help testing anything which is concerned with constant constructors?

If there is a need to change anything then we can do it in a new PR, I'll land this one now.

@eernstg eernstg merged commit ce14e53 into dart-lang:master Jan 8, 2025
2 checks passed
@sgrekhov
Copy link
Contributor Author

sgrekhov commented Jan 8, 2025

I was just wondering about one thing: The tests starting with ...A03_t06... seem to differ by having different kinds of parameters (optional named, required named). I couldn't see an obvious reason why this would help testing anything which is concerned with constant constructors?

There are 3 statements in the specification, about constructor with no optional parameters, with optional positional parameters and with named parameters. ...A03... tests checking mixin application constructor with named parameters.
...t06: optional named parameter, mixin has a static variable, mixin application of the form class MA = A with M;
...t07: required named parameter, mixin has a static variable, mixin application of the form class MA = A with M;
...t08: optional named parameter, mixin has a static variable, mixin application of the form class MA extends A with M...
...t09: required named parameter, mixin has a static variable, mixin application of the form class MA extends A with M...
and 4 more test but with the instance variable in a mixin.

I believe it makes sense to check all combinations. What if mixin application constructor looses its constantness, say, in case of required named parameter only? See, for example, dart-lang/sdk#59796.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 10, 2025
2025-01-10 [email protected] Fixes dart-lang/co19#3039. Fix the new roll failures (dart-lang/co19#3040)
2025-01-10 [email protected] dart-lang/co19#2976. Add constant expressions tests. Part 4. (dart-lang/co19#3037)
2025-01-09 [email protected] dart-lang/co19#2976. Add constant expressions tests (dart-lang/co19#3024)
2025-01-09 [email protected] Fixes dart-lang/co19#3030. Add additional test checking non const in a const constructor initializer list (dart-lang/co19#3036)
2025-01-09 [email protected] dart-lang/co19#2976. Add grammar tests for shorthand with different operators. (dart-lang/co19#3027)
2025-01-08 [email protected] Fixes dart-lang/co19#3028. Add tests for mixin application constant constructor (dart-lang/co19#3032)
2025-01-08 [email protected] dart-lang/co19#2976. Add constant expressions tests.Part 3. (dart-lang/co19#3026)
2025-01-08 [email protected] dart-lang/co19#2976. Add constant expressions tests.Part 2. (dart-lang/co19#3025)
2025-01-06 [email protected] dart-lang/co19#2976. Add more equality tests (dart-lang/co19#3023)
2025-01-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-java from 4.5.0 to 4.6.0 in the github-actions group (dart-lang/co19#3033)
2024-12-20 [email protected] dart-lang/co19#2976. Add more tests for shorthand equality (dart-lang/co19#3022)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: Ic8becbc8de5c560f3e9294a47d727442df7c733f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/403982
Reviewed-by: Erik Ernst <[email protected]>
Reviewed-by: Alexander Thomas <[email protected]>
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.

2 participants