Emit full 'IMapChangedEventArgsMethods' types for args in 'cswinrtgen' #2167
Conversation
This file contains hidden or 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
Renamed the 'currentMethod' variable to 'get_KeyMethod' in the IMapChangedEventArgs1Impl factory to better reflect its purpose and improve code clarity.
Inserted the Conv_U opcode in the IL instruction sequence within InteropTypeDefinitionBuilder.IEnumerator1. This change likely ensures correct type conversion for native interop scenarios.
Replaced inline method signature definitions for IMapChangedEventArgs`1 with calls to dedicated factory methods in WellKnownTypeSignatureFactory. This improves maintainability and centralizes signature creation logic.
Moved the generation of the 'Key' method for IMapChangedEventArgs1 to a new InteropMethodDefinitionFactory.IMapChangedEventArgs1Methods class. The new implementation provides a complete method body and integrates with emit state for return value rewriting. Updated all call sites to use the new factory method and pass the emit state.
Updated IAsyncInfoMethods to use get_UntypedRetVal instead of get_TypedRetVal for calli signatures. Removed the unused get_TypedRetVal method from WellKnownTypeSignatureFactory to simplify the code and avoid unnecessary type specificity.
Inserted the Conv_U opcode in the IL instruction sequence for KeyValuePair methods to ensure correct type conversion. This change improves the accuracy of the generated interop method definitions.
Split HasCurrentOrMoveNext into get_HasCurrent and MoveNext methods for clarity and maintainability. Updated call sites to use the new methods and restructured method bodies for GetMany and HasCurrentOrMoveNext to better match their intended signatures and logic.
There was a problem hiding this comment.
Pull request overview
This PR enhances the code generation for IMapChangedEventArgs<K> interfaces in cswinrtgen by emitting full method types instead of incomplete implementations. The changes include refactoring method signature creation, adding missing conv.u IL instructions for pointer conversions, and improving method organization.
Key Changes:
- Implemented complete code generation for
IMapChangedEventArgs<K>.Keymethod with proper marshaling and exception handling - Refactored method signature factories to eliminate duplicate code and improve reusability
- Added missing
conv.uinstructions for address-to-native integer pointer conversions
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| InteropGenerator.Emit.cs | Added emitState parameter to DefineIMapChangedEventArgsTypes call |
| WellKnownTypeSignatureFactory.cs | Removed duplicate get_TypedRetVal method and added specialized IMapChangedEventArgs1get_CollectionChangeImpl method |
| WellKnownTypeDefinitionFactory.cs | Refactored to use new factory methods instead of inline signature creation |
| InteropMethodDefinitionFactory.KeyValuePairMethods.cs | Added missing conv.u instruction for pointer conversion |
| InteropMethodDefinitionFactory.IMapChangedEventArgs1Methods.cs | New file implementing complete Key method with proper IL generation |
| InteropMethodDefinitionFactory.IMapChangedEventArgs1Impl.cs | Renamed variables and updated comments for consistency |
| InteropMethodDefinitionFactory.IEnumerator1Impl.cs | Reorganized methods, added explicit get_HasCurrent and MoveNext methods, and removed unused import |
| InteropMethodDefinitionFactory.IAsyncInfoMethods.cs | Replaced specialized signature call with generic get_UntypedRetVal |
| InteropTypeDefinitionBuilder.IMapChangedEventArgs1.cs | Replaced placeholder implementation with factory method call |
| InteropTypeDefinitionBuilder.IEnumerator1.cs | Updated method calls to use new explicit factory methods and added missing conv.u |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/WinRT.Interop.Generator/Factories/InteropMethodDefinitionFactory.IEnumerator1Impl.cs
Show resolved
Hide resolved
src/WinRT.Interop.Generator/Factories/InteropMethodDefinitionFactory.IEnumerator1Impl.cs
Show resolved
Hide resolved
Updated the HasCurrentOrMoveNext method to take an explicit adapterMethod parameter instead of determining it internally. This change clarifies method usage and improves code maintainability by making dependencies explicit.
Sergio0694
force-pushed
the
user/sergiopedri/map-changed-event-args-methods
branch
from
1e2158a to
e0a14ec
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Title.
Also added some missing
conv.uinstructions and did some minor refactoring.Examples of codegen:
Note
Draft because there's some commits to move and this branch also needs to be rebased.