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

Implement IOperation support for inline array access #68373

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Implement IOperation support for inline array access #68373

merged 1 commit into from
Jun 7, 2023

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented May 30, 2023 •
edited by jcouv
Loading

Relates to test plan #67826

@AlekseyTs AlekseyTs requested review from jjonescz and cston May 30, 2023 00:11
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 30, 2023
@AlekseyTs AlekseyTs marked this pull request as ready for review May 30, 2023 01:23
@AlekseyTs AlekseyTs requested review from a team as code owners May 30, 2023 01:23
@AlekseyTs
Copy link
Contributor Author

@cston, @dotnet/roslyn-compiler Please review

333fred
333fred reviewed May 30, 2023
View reviewed changes
src/Compilers/Core/Portable/Operations/OperationInterfaces.xml
@@ -3366,4 +3366,25 @@
</Comments>
</Property>
</Node>
<Node Name="IInlineArrayAccessOperation" Base="IOperation" ChildrenOrder="Instance,Argument" HasType="true">
Copy link
Member

@333fred 333fred May 30, 2023 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

I'm not sure why we couldn't simply reuse IArrayElementReferenceOperation for this? And even if we don't, we should probably keep the naming consistent. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why we couldn't simply reuse IArrayElementReferenceOperation for this?

I will be happy to have this discussion during an API review meeting. Personally, I don't feel that reusing is appropriate, given that the node doesn't always represent an element access.

And even if we don't, we should probably keep the naming consistent.

I think that there is more similarities with IImplicitIndexerReferenceOperation and I kept the naming consistent with that node. Again, we will review the shape of the API later.

@AlekseyTs
Copy link
Contributor Author

@cston, @dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Contributor Author

@333fred, @dotnet/roslyn-compiler Please review.

@AlekseyTs
Copy link
Contributor Author

@jjonescz, @333fred, @dotnet/roslyn-compiler Please review.

@AlekseyTs AlekseyTs merged commit 644a8f8 into dotnet:features/InlineArrays Jun 7, 2023
@allisonchou allisonchou mentioned this pull request Jun 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred left review comments

@cston cston cston approved these changes

@jjonescz jjonescz jjonescz approved these changes

Assignees
No one assigned
Labels
Area-Compilers New Feature - Inline Arrays untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlekseyTs @333fred @jjonescz @cston