Skip to content

Fix consume ValueTask backed by IValueTaskSource #2108

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
Merged
merged 6 commits into from
Mar 19, 2024
Merged

Fix consume ValueTask backed by IValueTaskSource #2108

merged 6 commits into from
Mar 19, 2024

Conversation

@timcassell
Copy link
Collaborator

@timcassell timcassell commented Sep 20, 2022

Fixes #1595.

This was originally #1941, but I split it out to just include the ValueTask fixes (and I added more tests).

This was referenced Sep 20, 2022
Merged
Closed
Open
Closed
@timcassell timcassell changed the title Fix consume ValueTask Fix consume ValueTask backed by IValueTaskSource Sep 20, 2022
@timcassell timcassell mentioned this pull request Sep 26, 2022
Open
@timcassell
Copy link
Collaborator Author

timcassell commented Jan 2, 2023

@adamsitnik Can you take a look?

@timcassell
Copy link
Collaborator Author

timcassell commented Jan 6, 2023

@AndreyAkinshin InProcessToolchain has been deprecated in favor of InProcessNoEmitToolchain since #1123 (almost 4 years ago). I updated it to match the changes I made here and in my other followup PRs, but I wonder if it can just be removed? Or should I just not add the updates and leave it in its old state? It's not good to maintain both.

@AndreyAkinshin
Copy link
Member

AndreyAkinshin commented Jan 6, 2023

I have no objection to removing InProcessToolchain. @adamsitnik what do you think?

@timcassell timcassell mentioned this pull request Jan 18, 2023
Merged
@timcassell timcassell force-pushed the fix-valuetask-only branch 2 times, most recently from 0dc5813 to 43138da Compare February 17, 2023 22:15
@timcassell
Copy link
Collaborator Author

timcassell commented Feb 17, 2023

@ig-sinicyn I touched the InProcess toolchains here, can you take a look?

@ig-sinicyn
Copy link
Contributor

ig-sinicyn commented Feb 18, 2023

@timcassell, great job!

There should be a diff test that proves that emitted IL matches with compiled c# code. If it passes, all should be fine.

@timcassell timcassell mentioned this pull request Jun 16, 2023
Closed
@timcassell timcassell force-pushed the fix-valuetask-only branch from 43138da to 13f5a0b Compare July 30, 2023 03:29
AndreyAkinshin
AndreyAkinshin approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

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

No concerns from my side. If @adamsitnik has no objections, we can merge it.

@timcassell timcassell merged commit 7306ee7 into dotnet:master Mar 19, 2024
@timcassell timcassell deleted the fix-valuetask-only branch March 19, 2024 22:08
@timcassell timcassell added this to the v0.14.0 milestone Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndreyAkinshin AndreyAkinshin AndreyAkinshin approved these changes

@adamsitnik adamsitnik Awaiting requested review from adamsitnik

+1 more reviewer

@ig-sinicyn ig-sinicyn ig-sinicyn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area:CodeGen

Projects

None yet

Milestone

v0.14.0

Development

Successfully merging this pull request may close these issues.

ValueTask-returning benchmarks are not being called correctly

3 participants

@timcassell @AndreyAkinshin @ig-sinicyn