-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Port CoreCLR's TypeNameBuilder to C#, and use it in Mono too #33701
Port CoreCLR's TypeNameBuilder to C#, and use it in Mono too #33701
Conversation
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
Would it make sense to switch CoreCLR to use the fully managed implementation as well and get rid of the mixed-mode implementation? |
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
ERROR = 0x0100, | ||
} | ||
|
||
ParseState m_parseState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseState does not seem to be used for anything. Delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean? CheckParseState
checks against this value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CheckParseState
checks this value, but the only thing that is done as a result of this test is setting this value to a different value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be debug-only and only used for asserts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan what to do with the parseState
? I would delete it completely. The asserts that it is used for are not very valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to keep it - The asserts helped me catch a porting typo in ToString, and they make some functions invariants explicit. I did remove the "error" state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it debug-only at least so that we do not have to carry it in shipping builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, make the names follow the C# coding style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I make this debug-only? Is using System.Diagnostics.Debug not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still going to be the enum type, an empty method and 10+ calls to this method, and the code that sets the field...
The test will not actually succeed with this PR, there are two other issues. Enabling it will not be part of the final PR |
It probably would, my first impression is that there isn't much use of this code from the native side (except perhaps those TypeString functions) |
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AQNBuilder.Mono.cs
Outdated
Show resolved
Hide resolved
Thank you for the feedback! This is a WIP:
|
In that case, it should follow the existing code style of shared SPC sources. I don't know what is the intention for CoreCLR but maybe it could land in the shared location instead of mono from the start |
3dfd0ee
to
c16b15c
Compare
Update:
|
Looks like System.Collections.Stack isn't in CoreCLR's CoreLib (while System.Collections.Generic.Stack is missing from Mono's) |
I do not see a reason for this split. Can all state and methods from TypeNameBuilderImpl be moved to be private state and methods on TypeNameBuilder ? |
b6faac4
to
6af771f
Compare
@jkotas I think all issues are resolved except how to keep the parser state checks. I like them because they were part of the original and this is still a port of that code, which has now changed considerably and could do with some checking. I also don't think they will have a real impact in a program's performance. But I understand they add some IL and an extra class to CorLib, so increase its size. Is the only way you would have them is in |
I can also ask the compiler for aggressive inlining for CheckParseState, to prevent the no-op calls on release. |
@marek-safar Are you ok with keeping debug-only ParserState cruft in release builds, or do you prefer to have it conditionally excluded? As I have said above, the value of these debug-only checks seems to be very low, and I believe it would be best to delete them. |
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
6af771f
to
d7d5962
Compare
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
…t/TypeNameBuilder.cs Co-Authored-By: Jan Kotas <[email protected]>
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
LGTM otherwise. Thanks! |
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Outdated
Show resolved
Hide resolved
…t/TypeNameBuilder.cs Co-Authored-By: Marek Safar <[email protected]>
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs
Show resolved
Hide resolved
897fa03
to
5434af8
Compare
5434af8
to
6abeeb4
Compare
Port CoreCLR's TypeNameBuilder to C#, and use it in Mono too
Mono's System.Reflection.Emit creates type names that fail to be normalized or shaped in all ways that CoreCLR does.
Port CoreCLR's mixed-mode thread-unsafe implementation to thread-safe C#, and start using it in Mono for names in TypeBuilder.
Fixes issues with e.g. null-delimited type names being passed to different Reflection.Emit builders. Contributes to #2389.