Add GetDeclaringType to PropertyDefinition and EventDefinition.
#111646
Conversation
|
Note regarding the |
|
Note regarding the |
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeSystem/PropertyDefinition.cs:72
- The new method GetDeclaringType should be covered by tests to ensure its correctness.
public TypeDefinitionHandle GetDeclaringType()
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.cs:1370
- Ensure that the new method
GetDeclaringTypeforPropertyDefinitionHandleis covered by tests.
return PropertyMapTable.FindTypeContainingProperty(propertyDef.RowId, PropertyTable.NumberOfRows);
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/MetadataReader.cs:1360
- Ensure that the new method
GetDeclaringTypeforEventDefinitionHandleis covered by tests.
return EventMapTable.FindTypeContainingEvent(eventDef.RowId, EventTable.NumberOfRows);
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeSystem/EventDefinition.cs:60
- The new method GetDeclaringType should be covered by tests to ensure it works as expected.
public TypeDefinitionHandle GetDeclaringType()
We have to return the value of the parent type column in the map table, not the row to the map table itself.
teo-tsirpanis
force-pushed
the
srm-declaring-type
branch
from
74a48f0 to
4779643
Compare
|
@dotnet/area-system-reflection-metadata this is ready for review. |
| int value = this.GetPropertyListStartFor(row); | ||
| if (value == propertyRowId) | ||
| { | ||
| while (row < numOfRows) | ||
| { | ||
| int newRow = row + 1; | ||
| value = this.GetPropertyListStartFor(newRow); | ||
| if (value == propertyRowId) | ||
| { | ||
| row = newRow; | ||
| } | ||
| else | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I do not think this is needed for property map table. Unlike typedef table that this code was copied from, the property map should not have rows for types with no properties.
Dtto for event map
| foreach (var typeDefHandle in reader.TypeDefinitions) | ||
| { | ||
| var typeDef = reader.GetTypeDefinition(typeDefHandle); | ||
| foreach (var nestedTypeHandle in typeDef.GetNestedTypes()) |
There was a problem hiding this comment.
Can we add a sanity check here to make sure that these foreach statement actually loop through at least one? Like:
Assert.True(typeDef.GetNestedTypes().Length > 0);
Fixes #60380. Implementation follows the corresponding methods for method and field definitions, except that the binary search is performed on rows of the
PropertyMapandEventMaptables for properties and fields respectively.