-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add EnumBuilder implementation and other changes #88503
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection-emit Issue DetailsContributes to https://github.com/dotnet/runtime/issues/ Tested AB.Save implementation with TlbImp3.exe with a sample *.tlb files. Adding/updating following based on the test result:
|
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/EnumBuilderImpl.cs
Outdated
Show resolved
Hide resolved
…t/EnumBuilderImpl.cs Co-authored-by: Jan Kotas <[email protected]>
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.
LGTM; some minor nits\questions.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/FieldBuilderImpl.cs
Outdated
Show resolved
Hide resolved
<data name="Argument_ConstantDoesntMatch" xml:space="preserve"> | ||
<value>Constant does not match the defined type.</value> | ||
</data> | ||
<data name="Argument_ConstantNull" xml:space="preserve"> |
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 wonder if we need the differentiation here - the one above may be acceptable for null as well, plus considering there is only one case for the null check.
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.
Yes, the above acceptable, but it might be cleared than the above, also the exception messages and throwing logic copied from corelib, it might be better to be consistent with corelib messaging, let me know if you think otherwise.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeNameBuilder.cs
Show resolved
Hide resolved
.../System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveCustomAttributeTests.cs
Outdated
Show resolved
Hide resolved
All failures are not related and known |
|
||
namespace System.Reflection.Emit | ||
{ | ||
internal sealed class TypeNameBuilder |
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.
This file is line-to-line identical to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs except for deleted comment and unused method.
Would it be better to just include the CoreLib in the .csproj file instead of copying it the file over?
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.
This file is line-to-line identical to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeNameBuilder.cs except for deleted comment and unused method.
Right, and I mentioned that in the description.
Would it be better to just include the CoreLib in the .csproj file instead of copying it the file over?
Was not aware of that was an option, I will try that out with my next PR which probably will not happen in .NET 8.0, would you like to update that within .NET 8?
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.
We have number of places where we borrow CoreLib code to avoid copying it around. Look for <Compile Include="$(CoreLibSharedDir)System...
. This pattern is typically used for down-level configs, I do not see a problem with doing the same here.
This does not need to be in .NET 8. However, I do prefer smaller more frequent PRs that do just one thing - they are much easier to review and merge.
Contributes to #62956
Tested AB.Save implementation with TlbImp3.exe with a sample *.tlb files. Adding/updating following based on the test result:
TypeBuilderImpl.FullName
( this neededTypeNameBuilder
which copied from Corelib)TypeBuilderImpl.CreateTypeInfoCore()
- for now just returning theTypeBuilderImp
itself, might need some update after IL implementation added;EnumBuiderImpl
, mostly usingTypeBuilderImpl
code for writing to fileEnumBuiderImpl
needsFieldBuilderImp.SetConstantCore(object? defaultValue)
implementation, so added it