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

Initial work to support EnumeratorCancellation. #888

Merged
merged 7 commits into from
Sep 29, 2020
Merged

Conversation

bartdesmet
Copy link
Collaborator

@bartdesmet bartdesmet commented Apr 22, 2019

See:

This PR adds the attribute for downlevel platforms, and leverages it for IAsyncEnumerable<T>-based iterators where the Create(Core) pattern using an IAsyncEnumerator<T>-based iterator has been used. The result will be reduction of allocations.

Goal is to ship with HAS_ASYNC_ENUMERABLE_CANCELLATION always set (and hence #if checks removed). Right now, the flag has to be set explicitly because we don't yet have a compiler drop that picks up on this attribute.

When we prepare for the final release in concert with C# 8.0 and .NET Core 3.0 shipping, we'll also revisit the #if USE_ASYNC_ITERATOR checks; we should be able to get rid of a lot of hand-rolled state machines then.

}
}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

I think we need an #else with a TypeForwardedTo here to unify like the rest of the types.

@codecov
Copy link

codecov bot commented Apr 22, 2019

Codecov Report

❗ No coverage uploaded for pull request base (main@161e4ad). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #888   +/-   ##
=======================================
  Coverage        ?   63.62%           
=======================================
  Files           ?      165           
  Lines           ?    25734           
  Branches        ?    18786           
=======================================
  Hits            ?    16372           
  Misses          ?     2138           
  Partials        ?     7224           
Flag Coverage Δ
#ixnet 63.62% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 161e4ad...dc1a66c. Read the comment docs.

@bartdesmet
Copy link
Collaborator Author

bartdesmet commented May 24, 2019

This is currently blocked on the lack of support for custom attributes on local functions, see dotnet/csharplang#1888. The LDM decision is to add support for these in order to unblock this; see https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-04-29.md.

We realized that attributes are not permitted on local function parameters, which means that you cannot use the EnumeratorCancellation attribute in local function iterators.

Conclusion
Allow attributes on local function parameters and type parameters.

Also note that if we merge this PR, we should consider converting some of these local functions to be static to avoid an additional allocation. Other than the "local" nature of local functions (thus not having to come up with a name for another method), we'd pretty much not be using any of the features of local functions (closure, delegate conversion), so if we really care deeply about optimizing these query operators before we get custom attributes on local functions, we could turn them into non-local functions (with *Core method names).

@dnfadmin dnfadmin changed the base branch from master to main September 26, 2020 18:56
@bartdesmet bartdesmet merged commit 8d279c6 into main Sep 29, 2020
@bartdesmet bartdesmet deleted the IxAsyncCancellation branch September 29, 2020 03:19
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