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.

Sign up for GitHub

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

Jump to bottom

Avoid creating result temp for is-expressions (follow-up) #72273

Merged
merged 13 commits into from
Mar 22, 2024
Merged

Avoid creating result temp for is-expressions (follow-up) #72273

merged 13 commits into from
Mar 22, 2024

Conversation

alrz
Copy link
Contributor

@alrz alrz commented Feb 26, 2024 •
edited
Loading

Follow-up to #68694 including a possible fix for #69615 at 9ed6e9d.

In the repro, I traced the difference between iterating over List<T> and ImmutableArray<T> where the former works correctly and the enumerator is captured in a field. For List<T> the loop is wrapped in a try-finally where the state is modified afterwards. Looking back at the VisitLoweredIsPatternExpression I noticed this could be a possible missing piece.

Closes #59615
Closes #55334

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 26, 2024
@alrz alrz mentioned this pull request Feb 27, 2024
Draft
@alrz alrz marked this pull request as ready for review March 5, 2024 21:52
@alrz alrz requested a review from a team as a code owner March 5, 2024 21:52
@alrz alrz requested a review from AlekseyTs March 5, 2024 21:52
@jcouv jcouv self-assigned this Mar 5, 2024
@AlekseyTs
Copy link
Contributor

@alrz How was the first commit in this PR created?

@alrz
Copy link
Contributor Author

alrz commented Mar 7, 2024 •
edited
Loading

@alrz How was the first commit in this PR created?

That's the same squash I made to open #69194 before the original PR got merged, but no commits were added after that.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 7, 2024 •
edited
Loading

@alrz How was the first commit in this PR created?

That's the same squash I made to open #69194 before the original PR got merged, but no commits were added after that.

Could you please instead replace it with revert of revert of the original merged PR? I am referring to #68694 and to #69582 that followed.

@alrz
Copy link
Contributor Author

alrz commented Mar 7, 2024

Could you please instead replace it with revert of revert of the original merged PR?

Done. Let me know if the rest should be squashed if you intend to keep the revert in the merge.

@alrz
Copy link
Contributor Author

alrz commented Mar 7, 2024

image
That's not helpful. I don't know since when git try to use "reapply" when reverting a revert.

@AlekseyTs
Copy link
Contributor

Could you please instead replace it with revert of revert of the original merged PR?

Done.

Thanks!

Let me know if the rest should be squashed if you intend to keep the revert in the merge.

No. We will squash while merging this PR

@alrz
Copy link
Contributor Author

alrz commented Mar 7, 2024

Sure, thought you wanted to do that separately. btw I made the rebase branch to rebase the rest of commits but ended up using --onto HEAD and resetting to the original for push. A little misleading.

AlekseyTs
AlekseyTs approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 9), assuming CI is passing

@AlekseyTs
Copy link
Contributor

@jcouv, @dotnet/roslyn-compiler For the second review.

@alrz alrz requested review from jcouv and a team March 14, 2024 23:12
@AlekseyTs
Copy link
Contributor

@jcouv, @dotnet/roslyn-compiler For the second review.

jcouv
jcouv reviewed Mar 20, 2024
View reviewed changes
src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
EmitCondExpr(sequence.Value, sense: sense);
}
else
{
Copy link
Member

Choose a reason for hiding this comment

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

Can we assert that sense == true in this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an assertion to the same effect.

Copy link
Member

Choose a reason for hiding this comment

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

We can get into the alternative branch with !used and a boolean type. The added assertion doesn't cover sense in that scenario.

jcouv
jcouv reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 9). Only one minor question/suggestion

AlekseyTs
AlekseyTs approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 12)

jcouv
jcouv reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 12). Still just a nit on assertion

@alrz alrz requested a review from jcouv March 22, 2024 08:28
jcouv
jcouv approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 13)

@AlekseyTs AlekseyTs merged commit 00c20fc into dotnet:main Mar 22, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 22, 2024
@AlekseyTs
Copy link
Contributor

@alrz Thanks for the contribution

@RikkiGibson RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024
@dotnet-bot dotnet-bot mentioned this pull request Mar 26, 2024
Closed
@jcouv jcouv mentioned this pull request Apr 1, 2024
Closed
jcouv added a commit to jcouv/roslyn that referenced this pull request Apr 1, 2024
@jcouv
Revert "Avoid creating result temp for is-expressions (dotnet#72273)"
b9d7ae7 
This reverts commit 00c20fc.
@jcouv jcouv mentioned this pull request Apr 1, 2024
Merged
jcouv added a commit that referenced this pull request Apr 2, 2024
@jcouv
Revert "Avoid creating result temp for is-expressions (#72273)" (#72827)
df7f62f 
This reverts commit 00c20fc.
jcouv added a commit to jcouv/roslyn that referenced this pull request Apr 2, 2024
@jcouv
Revert "Avoid creating result temp for is-expressions (dotnet#72273)" (…
2aeb129 
…dotnet#72827)

This reverts commit 00c20fc.
jaredpar pushed a commit that referenced this pull request Apr 2, 2024
@jcouv
Revert "Avoid creating result temp for is-expressions (#72273)" (#72827…
237ce08 
…) (#72845)

This reverts commit 00c20fc.
333fred added a commit to 333fred/roslyn that referenced this pull request Apr 3, 2024
@333fred
Merge remote-tracking branch 'upstream/main' into ref-struct-dynamic
21e1049 
* upstream/main: (416 commits)
  Semantic search (dotnet#71268)
  Make more static
  Fix MEF import of IExternalCSharpCopilotCodeAnalysisService to allow null
  Make static
  Make private
  Add comments
  Add method name to TimeInQueue telemetry (dotnet#72841)
  switch to frozen
  Simplify
  Add test
  Downstream
  Use singular helper when creating checksumsw
  Use singular helper when creating checksumsw
  Remove ability for a project to change its language
  Revert "Avoid creating result temp for is-expressions (dotnet#72273)" (dotnet#72827)
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Improve generic type argument list error recovery (dotnet#69734)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.10 P3
4 participants
@alrz @AlekseyTs @jcouv @RikkiGibson