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

Abstract out LocalBuilder, emit Locals with new ILGenerator #93809

Merged
merged 16 commits into from Oct 27, 2023
Merged

Abstract out LocalBuilder, emit Locals with new ILGenerator #93809

merged 16 commits into from Oct 27, 2023

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Oct 21, 2023 •
edited

  • Abstract out LocalBuilder
  • Added implementation for DeclareLocal(Type localType, bool pinned), (OpCode opcode, LocalBuilder local), and corresponding tests

Contributes to #92975
Fixes #93497

@ghost ghost assigned buyaa-n Oct 21, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Oct 21, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Abstract out LocalBuilder (blocked until the API proposal approved)
  • Added implementation for DeclareLocal(Type localType, bool pinned), (OpCode opcode, LocalBuilder local), and corresponding tests

The PR has commits from #93244, will be cleaned up after #93244 merged.

Contributes to #92975

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-System.Reflection.Emit, new-api-needs-documentation
Milestone: -

@buyaa-n buyaa-n mentioned this pull request Oct 21, 2023
Closed
20 tasks
This was referenced Oct 21, 2023
Closed
Closed
Closed
@buyaa-n buyaa-n marked this pull request as ready for review October 25, 2023 23:02
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Oct 26, 2023
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/LocalBuilder.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit.ILGeneration/tests/LocalBuilder/LocalBuilderMethod.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit.ILGeneration/tests/LocalBuilder/LocalBuilderMethod.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ILGeneratorImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ILGeneratorImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/LocalBuilderImpl.cs Outdated Show resolved Hide resolved
src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeLocalBuilder.Mono.cs Outdated Show resolved Hide resolved
@buyaa-n @AaronRobinsonMSFT
Apply suggestions from code review
2cd5a84 
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@build-analysis build-analysis bot mentioned this pull request Oct 26, 2023
Open
@buyaa-n buyaa-n mentioned this pull request Oct 26, 2023
Closed
This was referenced Oct 26, 2023
Closed
Open
@buyaa-n @jkotas
Avoid invalid cast exception.
47b436b 
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
jkotas
jkotas approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

This was referenced Oct 27, 2023
Closed
Open
@buyaa-n
Copy link
Member Author

buyaa-n commented Oct 27, 2023

Thank you for the review! All failures are known.

@buyaa-n buyaa-n merged commit ce1264c into dotnet:main Oct 27, 2023
172 of 176 checks passed
@buyaa-n buyaa-n deleted the local branch October 27, 2023 15:38
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
@buyaa-n @AaronRobinsonMSFT
@jkotas @liveans
Abstract out LocalBuilder, emit Locals with new ILGenerator (dotnet#9…
de69e83 
…3809)

* Abstract LocalBuilder, emit LocalBuilder in ILGenerator

* Apply suggestions from code review

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

* Remove public LocalBuilder.Method, and apply other feedback

* Avoid invalid cast exception.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

---------

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@steveharter steveharter Awaiting requested review from steveharter

@lambdageek lambdageek Awaiting requested review from lambdageek

Assignees

@buyaa-n buyaa-n

Labels
area-System.Reflection.Emit new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add protected factory method for creating Label in ILGenerator, abstract out LocalBuilder
4 participants
@buyaa-n @lambdageek @jkotas @AaronRobinsonMSFT