Automatically port Reflection.PortableExecutable.DllCharacteristics #3195
Conversation
carlossanlop
added
new-content
carlossanlop
requested review from
GrabYourPitchforks,
steveharter,
rpetrusha and
mairaw
There was a problem hiding this comment.
I've left a couple of comments and suggestions, @carlossanlop.
| @@ -49,7 +49,7 @@ | |||
| </ReturnValue> | |||
There was a problem hiding this comment.
Can we add a type summary, something like "Describes the characteristics of a dynamic link library."?
There was a problem hiding this comment.
The type summary sounds good to me, I will apply it.
@GrabYourPitchforks @steveharter as the area owners, do you agree with the type summary?
There was a problem hiding this comment.
Actually, @tmat please take a look at this PR as well.
| @@ -348,7 +348,7 @@ | |||
| </ReturnValue> | |||
| <MemberValue>8192</MemberValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <summary>Driver uses WDM model.</summary> | |||
There was a problem hiding this comment.
| <summary>Driver uses WDM model.</summary> | |
| <summary>The driver uses the WDM model.</summary> |
| @@ -248,7 +248,7 @@ | |||
| </ReturnValue> | |||
| <MemberValue>2</MemberValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <summary>Reserved.</summary> | |||
| </Docs> | |||
| </Member> | |||
| <Member MemberName="TerminalServerAware"> | |||
There was a problem hiding this comment.
Do we have a description for TerminalServerAware?
There was a problem hiding this comment.
Not in the source comments: src/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEFileFlags.cs#L113
This enum value was introduced in the very first commit of .NET Core: dotnet/corefx@f11305d
So the API is as old as Framework. But judging by the value, and a quick search I did for these terms, this is my proposed description, @rpetrusha:
The image is Terminal Server aware.
@GrabYourPitchforks @steveharter as the area owners, is this description good enough for you?
There was a problem hiding this comment.
Seems reasonable to me, but I am not an expert.
| @@ -74,7 +74,7 @@ | |||
| </ReturnValue> | |||
| <MemberValue>64</MemberValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <summary>DLL can move.</summary> | |||
There was a problem hiding this comment.
| <summary>DLL can move.</summary> | |
| <summary>The DLL can move.</summary> |
| @@ -74,7 +74,7 @@ | |||
| </ReturnValue> | |||
| <MemberValue>64</MemberValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <summary>The DLL can move.</summary> | |||
There was a problem hiding this comment.
This doesn't seem to make sense.
There was a problem hiding this comment.
Maybe it should be The DLL can be relocated.
| @@ -248,7 +248,7 @@ | |||
| </ReturnValue> | |||
| <MemberValue>2</MemberValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <summary>Reserved.</summary> | |||
| </Docs> | |||
| </Member> | |||
| <Member MemberName="TerminalServerAware"> | |||
There was a problem hiding this comment.
Seems reasonable to me, but I am not an expert.
|
I think that this is ready to merge, @carlossanlop. I've changed "The DLL can move" to "The DLL can be relocated." I've approved, so let me know if it's ready to merge. |
rpetrusha
removed
the
waiting-on-reviews
|
Your last PR is ready to merge, @rpetrusha :) |
|
Thanks, @carlossanlop. I'll merge now. |
No description provided.