Add InstructionEncoder.Switch.
#76526
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The opcode is already stored in a field. And it's no more stored as a byte; it takes the same space as an ILOpCode due to packing.
…rolFlowBuilder. It simplifies the control flow builder and paves the way to support the switch instruction in the future. As a drawback the BranchInfo struct became bigger due to fields that are necessary for error reporting.
…lder. There didn't seem to be any reason to use them; they were not converted to immutable arrays.
And rename instructionStartOffset to ilOffset.
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata Issue DetailsFixes #40546. This PR refactors I also added two tests.
|
teo-tsirpanis
force-pushed
the
srm-switch
branch
2 times, most recently
from
4b68314
to
c44f173
Compare
This was referenced Oct 10, 2022
Closed
buyaa-n
reviewed
Oct 11, 2022
.../System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/EnumerableExtensions.cs
Outdated
Show resolved
Hide resolved
buyaa-n
reviewed
Oct 11, 2022
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs
Show resolved
Hide resolved
buyaa-n
reviewed
Oct 11, 2022
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/InstructionEncoder.cs
Outdated
Show resolved
Hide resolved
buyaa-n
reviewed
Oct 11, 2022
...raries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/ControlFlowBuilderTests.cs
Show resolved
Hide resolved
…ile switch branches are being emitted.
teo-tsirpanis
force-pushed
the
srm-switch
branch
from
c44f173
to
d5c7793
Compare
teo-tsirpanis
commented
Oct 13, 2022
src/libraries/System.Reflection.Metadata/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
tmat
reviewed
Oct 13, 2022
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs
Outdated
Show resolved
Hide resolved
buyaa-n
approved these changes
Oct 14, 2022
This reverts commit 92a1f80.
steveharter
reviewed
Oct 26, 2022
src/libraries/System.Reflection.Metadata/ref/System.Reflection.Metadata.cs
Show resolved
Hide resolved
steveharter
reviewed
Oct 26, 2022
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs
Show resolved
Hide resolved
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/ControlFlowBuilder.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Nov 3, 2022
src/libraries/System.Reflection.Metadata/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Metadata/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
.../System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/EnumerableExtensions.cs
Outdated
Show resolved
Hide resolved
.../System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/EnumerableExtensions.cs
Outdated
Show resolved
Hide resolved
...em.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/InstructionEncoder.cs
Show resolved
Hide resolved
...lection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/SwitchInstructionEncoder.cs
Outdated
Show resolved
Hide resolved
...raries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/InstructionEncoderTests.cs
Show resolved
Hide resolved
...raries/System.Reflection.Metadata/tests/Metadata/Ecma335/Encoding/InstructionEncoderTests.cs
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
|
Any more feedback on this? |
LGTM - do you have permission to do the merge? |
|
No, I am not a team member. 😅 |
|
Thanks! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #40546.
This PR refactors
System.Reflection.Metadata.ControlFlowBuilderso that instead of tracking branch instructions, it tracks just the label operands. This allows instructions with more than one label operand such asswitch, which was implemented according to the approved API shape.I also added two tests.