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

Implement design changes for "pattern" Index/Range indexers #34897

Merged
merged 7 commits into from Apr 12, 2019

Conversation

Projects
None yet
4 participants
@agocke
Copy link
Contributor

agocke commented Apr 10, 2019

Implements most of the design changes specified in
https://github.com/dotnet/csharplang/blob/c229cae634bd59a6a13b9ed464a4cab782a95e5d/proposals/index-range-changes.md

This PR focuses on getting the simple end-to-end scenario working,
not focusing entirely on codegen quality. I expect to follow-up later
with the optimizations mentioned about eliminating use of the
Index/Range helpers entirely if they can be elided.

@agocke agocke force-pushed the agocke:pattern-index-range branch 2 times, most recently from 0c0e1d4 to 2ebfc58 Apr 10, 2019

@agocke agocke changed the title WIP Implement design changes for "pattern" Index/Range indexers Apr 10, 2019

Implement design changes for "pattern" Index/Range indexers
Implements most of the design changes specified in
https://github.com/dotnet/csharplang/blob/c229cae634bd59a6a13b9ed464a4cab782a95e5d/proposals/index-range-changes.md

This PR focuses on getting the simple end-to-end scenario working,
not focusing entirely on codegen quality. I expect to follow-up later
with the optimizations mentioned about eliminating use of the
Index/Range helpers entirely if they can be elided.

@agocke agocke force-pushed the agocke:pattern-index-range branch from 2ebfc58 to ae2c987 Apr 10, 2019

@agocke agocke marked this pull request as ready for review Apr 10, 2019

@agocke agocke requested a review from dotnet/roslyn-compiler as a code owner Apr 10, 2019

@agocke agocke added this to the 16.1.P2 milestone Apr 10, 2019

@agocke agocke requested a review from jaredpar Apr 10, 2019

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Apr 10, 2019

@333fred Could you take a look at the IOperation stuff and see if it's good enough for a first pass?

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Apr 11, 2019

Done with review pass (iteration 1).

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Apr 11, 2019

Done with review pass (iteration 2). Mostly small comments.

agocke added some commits Apr 11, 2019

/// <summary>
/// The required name for the <c>Count</c> property used in a pattern-based Index or Range indexer.
/// </summary>
public const string CountPropertyName = "Count";

This comment has been minimized.

Copy link
@AlekseyTs

AlekseyTs Apr 11, 2019

Contributor

public const string CountPropertyName = "Count"; [](start = 8, length = 48)

Why would we want to expose these constants publicly?

This comment has been minimized.

Copy link
@agocke

agocke Apr 12, 2019

Author Contributor

Great question! I don't know, but it seems like all of these are public. Why should these members be different?

This comment has been minimized.

Copy link
@AlekseyTs

AlekseyTs Apr 12, 2019

Contributor

Why should these members be different?

Why should we have these members in this type?


In reply to: 274721942 [](ancestors = 274721942)

This comment has been minimized.

Copy link
@agocke

agocke Apr 12, 2019

Author Contributor

It's where we store the names for other patterns, like foreach or async dispose. I could put them elsewhere, this just seemed like the natural place.

@agocke

This comment has been minimized.

Copy link
Contributor Author

agocke commented Apr 12, 2019

@jaredpar @AlekseyTs Any further comments?

indexerAccessExpression = BadIndexerExpression(node, expr, analyzedArguments, lookupResult.Error, diagnostics);
if (TryBindIndexOrRangeIndexer(
node,
expr,

This comment has been minimized.

Copy link
@RikkiGibson

RikkiGibson Apr 12, 2019

Contributor

We commonly style indexer access expressions as receiver[expr], so naming this expr here seems confusing.

This comment has been minimized.

Copy link
@agocke

agocke Apr 12, 2019

Author Contributor

Actually, indexers can have multiple arguments, so it would be receiver[arg1, arg2, arg3] so using expr as the argument name just means we're inserting an expression (which could be any of those components).

@agocke agocke merged commit 357fa7e into dotnet:master Apr 12, 2019

1 check passed

license/cla All CLA requirements met.
Details

@agocke agocke deleted the agocke:pattern-index-range branch Apr 12, 2019

333fred added a commit to 333fred/roslyn that referenced this pull request Apr 17, 2019

Merge remote-tracking branch 'dotnet/master' into merge-master
* dotnet/master: (495 commits)
  Roslyn Installer: Stop processes that block VSIX installation. (dotnet#34886)
  Remove unused helper BeginInvokeOnUIThread
  Apply a hang mitigating timeout to InvokeOnUIThread
  Apply a hang mitigating timeout in RestoreNuGetPackages
  Apply a hang mitigating timeout to WaitForApplicationIdle
  Fix Value/Ref checks for pattern Index/Range (dotnet#35004)
  Fix assert in remove unused member analyzer
  Treat unconstrained type parameters declared in `#nullable disable` context as having an oblivious nullability in case they are substituted with a reference type. (dotnet#34797)
  Add symbol name to tests. Fix to be the correct name emitted
  PR feedback
  Improve IDE0052 diagnostic message for properties with used setter but unused getter
  Use original definition symbol for fetching control flow graph of generic local functions.
  Properly treat ambiguous explicit interface implementations (dotnet#34584)
  Remove the dependence between the order in NullableAnnotation and metadata attribute values (dotnet#34909)
  Fix warning level test.
  Fix bootstrap on Linux/Mac (dotnet#34978)
  disable completion for immediate window commands (dotnet#32631)
  Updated PreviewWarning color
  Implement design changes for "pattern" Index/Range indexers  (dotnet#34897)
  Add IVTs to 16.1 version of RemoteLS
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.