Properly populate ExportedType metadata table for forwarded extension blocks. #80361
Conversation
AlekseyTs
added
Area-Compilers
Feature - Extension Everything
| return groupingTypes; | ||
| } | ||
|
|
||
| public sealed override ImmutableArray<Cci.ExportedType> GetExportedTypes(EmitContext context) |
There was a problem hiding this comment.
Consider passing EmitContext as in since it's a large structure. #Resolved
There was a problem hiding this comment.
Consider passing EmitContext as in since it's a large structure.
I'll stick to the current way of passing. As far as I can see in existing code, it is passed everywhere by value. If that becomes a real problem we would need to change all of them. Also, VB doesn't have a concept of in, but this method is also overridden in VB.
|
|
||
| while (stack.Count > currentStackSize) | ||
| { | ||
| processTopItemFromStack(stack, context, builder); |
There was a problem hiding this comment.
Why are we processing the newly pushed types here immediately instead of processing them in the top-level loop? #Resolved
There was a problem hiding this comment.
Why are we processing the newly pushed types here immediately instead of processing them in the top-level loop?
Because we want to process them before we append grouping and marker types. And we want to append grouping and marker types before processing anything else from the stack. That ensures consistent ordering across all different forms of extensions representation.
| using System.Reflection.Metadata; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading; | ||
| using Microsoft.Cci; |
There was a problem hiding this comment.
This using looks unnecessary if we use Cci. prefix in the code below. #Resolved
There was a problem hiding this comment.
This using looks unnecessary
IDE generated stubs for interface implementation and added this using in the process. That code doesn't use prefix.
| internal override string? ExtensionMarkerName | ||
| => _underlyingType.ExtensionMarkerName; | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
This seems superfluous - there is already #nullable enable a few lines above. #Pending
|
|
||
| internal ImmutableArray<(Cci.INestedTypeReference GroupingType, ImmutableArray<Cci.INestedTypeReference> MarkerTypes)> GetExtensionGroupingAndMarkerTypesForTypeForwarding(EmitContext context) | ||
| { | ||
| if (_lazyExtensionGroupingAndMarkerTypesForTypeForwarding.IsDefault) |
There was a problem hiding this comment.
Is it guaranteed that this won't be called twice with a different EmitContext (which could mean getting a stale cached value)? #Resolved
There was a problem hiding this comment.
Is it guaranteed that this won't be called twice with a different EmitContext
No, but the actual implementation of the called APIs doesn't use the context.
There was a problem hiding this comment.
That sounds fragile to me. Consider mentioning that in a comment on the implementation side.
|
|
||
| foreach (var groupingType in groupingTypes) | ||
| { | ||
| var retargetedGroupingType = new ForwardedExtensionGroupingOrMarkerType(this.GetCciAdapter(), groupingType); |
There was a problem hiding this comment.
| var retargetedGroupingType = new ForwardedExtensionGroupingOrMarkerType(this.GetCciAdapter(), groupingType); | |
| var forwardedGroupingType = new ForwardedExtensionGroupingOrMarkerType(this.GetCciAdapter(), groupingType); | |
| ``` #Resolved |
There was a problem hiding this comment.
The current name looks fine to me
|
|
||
| internal ImmutableArray<(Cci.INestedTypeReference GroupingType, ImmutableArray<Cci.INestedTypeReference> MarkerTypes)> GetExtensionGroupingAndMarkerTypesForTypeForwarding(EmitContext context) | ||
| { | ||
| if (_lazyExtensionGroupingAndMarkerTypesForTypeForwarding.IsDefault) |
There was a problem hiding this comment.
That sounds fragile to me. Consider mentioning that in a comment on the implementation side.
Closes #79894.
Relates to test plan #76130