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

List patterns: Add initial support for enumerables #66553

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alrz
Copy link
Member

@alrz alrz commented Jan 26, 2023

This deviates from the draft spec in a few ways, in particular:

  • Splitting TryGetElement into a pair of HasElementAt and GetElementFrom{Start,End} methods: To facilitate subsumption checking, a new test node is added to track the count. It narrows down a virtual temp by count > index at each element access. This also correctly eliminates forthcoming element tests where the result is known:

    • HasElementAt(i) implies HasElementAt(j) where i >= j
    • !HasElementAt(i) implies !HasElementAt(j) where i <= j

    The "relation condition" is inserted as a constant value test just like indexables, because count > 1 && !(count > 2) produced by a fixed-length pattern (see below) leads to a single remaining value. This test won’t reach codegen.

    It’s still possible to lower a sequence of (element test, element evaluation) as TryGetElement for a slightly smaller code size.

  • Removal of Last(): To test the end of the sequence, a negated test for the next element is used instead: !HasElementAt(N)

  • Removal of Count(): Trailing patterns are prefixed by HasElementAt(N-2) to ensure there’s sufficient elements before calling GetElementFromEnd for each subpattern after the slice.

Note: this only covers IEnumerable<T> at the moment.

@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 26, 2023
@jcouv jcouv self-assigned this Jan 26, 2023
@alrz
Copy link
Member Author

alrz commented Mar 11, 2023

This is ready for an initial review pass @jcouv @dotnet/roslyn-compiler
Had some questions, added as PROTOTYPE comments in the code.
Thanks.

(Some changes are pulled from #66481 and #66692)

@alrz alrz marked this pull request as ready for review March 11, 2023 15:17
@alrz alrz requested a review from a team as a code owner March 11, 2023 15:17
@jaredpar jaredpar added this to the C# 12.0 milestone Mar 20, 2023
@jaredpar
Copy link
Member

Thanks for contributing this as always @alrz 😄

At the moment we are a bit overbooked for work. I think the earliest we'll be able to book this for review is this summer. It is on our list of potentials though.

Sorry, this cycle has been a bit more resource constrained than anticipated.

@ChadNedzlek
Copy link
Member

Curious about the state of this, as it's been a couple years, and this issue is referenced in the regex group.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft January 24, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants