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

#2559. Add extension types to augmented expression tests. Part 7. #2966

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Nov 5, 2024

No description provided.

@sgrekhov sgrekhov requested review from eernstg, athomas and chloestefantsova and removed request for athomas November 5, 2024 09:02
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.

Looks good!

I think just one change would be useful (but it does seem to affect a bit of existing code as well): It is probably safer to avoid using augmented as an expression in a line which is supposed to give rise to an error because of the fact that augmented is also the name of a declaration in the same line.

The point is (as usual) that we shouldn't test derived errors, and it's not easy to say what's the correct handling (silently skip over, or report error for) a location where an ill-defined variable is used.

// ^^^^^^^^^
// [analyzer] unspecified
// [cfe] unspecified
for (int augmented = 0; augmented < 0; augmented++) {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe avoid using the declaration? We could have a bug whereby the declaration is accepted, but the usages of augmented as an expression give rise to an error, and then we'd have a false positive on this error. Perhaps the following would suffice:

Suggested change
for (int augmented = 0; augmented < 0; augmented++) {}
for (int augmented = 0;;) {}

Copy link
Member

Choose a reason for hiding this comment

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

(I guess this question wasn't raised when the non-new text was created, e.g., line 359)

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Nov 5, 2024

Makes sense! Updated. PTAL.

@sgrekhov sgrekhov requested a review from eernstg November 5, 2024 12:53
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.

Thanks! LGTM

@eernstg eernstg merged commit e0e3e18 into dart-lang:master Nov 5, 2024
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Nov 8, 2024
2024-11-06 [email protected] Fixes dart-lang/co19#2968. Fix expected result in syntax_relational_A01_t01.dart (dart-lang/co19#2969)
2024-11-06 [email protected] Fixes dart-lang/co19#2967. Update/ignore invalid_null_aware_operator warnings (dart-lang/co19#2970)
2024-11-05 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 7. (dart-lang/co19#2966)
2024-11-04 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 6. (dart-lang/co19#2965)
2024-11-04 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 5. (dart-lang/co19#2964)
2024-11-01 49699333+dependabot[bot]@users.noreply.github.com Bump the github-actions group with 2 updates (dart-lang/co19#2963)
2024-11-01 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 4. (dart-lang/co19#2962)
2024-11-01 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 3. (dart-lang/co19#2961)
2024-11-01 [email protected] dart-lang/co19#2138. Add more tests for functions with a short body (dart-lang/co19#2958)
2024-10-31 [email protected] dart-lang/co19#2559. Add extension types to `augmented` expression tests. Part 2. (dart-lang/co19#2960)
2024-10-31 [email protected] dart-lang/co19#2559. Add extension types to augmented expression tests. Part 1. (dart-lang/co19#2959)
2024-10-31 [email protected] dart-lang/co19#2559. Add `augmented` expression tests for control-flow elements (dart-lang/co19#2798)

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