Add support for properties #50
Conversation
|
I wasn't sure if there's a good way to add tests since we don't have the reflection infrastructure and nothing will currently mark the properties - so I just tested with |
| // Adding PropertyMap entries when writing types ensures that the PropertyMap table has the same | ||
| // order as the TypeDefinition table. This allows us to use the same logic in MapTypePropertyList | ||
| // as we have for fields and methods. | ||
| PropertyDefinitionHandle propertyHandle = writeContext.TokenMap.MapTypePropertyList(Handle); |
There was a problem hiding this comment.
Looks like we'll end up with PropertyMap entries for types with no properties. This is technically not allowed by the ECMA spec (also it's unnecessary bytes).
It would be nice if we could end up in a situation where only properties with a (marked) property end up with an entry in this table.
Maybe we should diverge from how Map[Field/Method]List is implemented and have this API return a nil token if there's no marked property on the type.
If the API returns nil, we skip the AddPropertyMap call.
There was a problem hiding this comment.
(By marked property, I mean a property with an assigned token in the output assembly - it's a question that TokenMap can answer.)
There was a problem hiding this comment.
Good point, I changed it to return nil if the type has no properties.
For AddPropertyMap I just realized this approach depends on the input PropertyMap table being in the same order as the TypeDef table. It doesn't look like this is guaranteed by the spec so I'm wondering if we need to track the PropertyMap tokens separately in a new PropertyMapNode. There doesn't seem to be a PropertyMapHandle so I suppose we'll need to manually walk the metadata tables. Does that sound reasonable to you?
There was a problem hiding this comment.
Where do we make an assumption that the PropertyMap table has the same order as TypeDef table?
src/coreclr/tools/ILTrim/ILTrim/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs
Show resolved
Hide resolved
src/coreclr/tools/ILTrim/ILTrim/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs
Outdated
Show resolved
Hide resolved
- Avoid creating PropertyMap rows for types without properties - Use GetAccessors to get property accessors
|
Should there be a testcase in Tests.Cases in addition to the the repro? |
|
Ideally yes, but see #50 (comment). |
src/coreclr/tools/ILTrim/ILTrim/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/ILTrim/ILTrim/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs
Outdated
Show resolved
Hide resolved
- Don't forget to map tokens!
Fixes #39