Automatically port Reflection.PortableExecutable.PE* APIs #3193

carlossanlop · 2019-09-16T19:00:40Z

No description provided.

rpetrusha

I've left a number of suggestions and comments about the first two files in your PR, @carlossanlop. I'll do the remainder separately.

rpetrusha · 2019-09-25T19:04:10Z

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml

@@ -86,7 +86,15 @@
      <Docs>
        <summary>To be added.</summary>
        <value>To be added.</value>
-        <remarks>To be added.</remarks>
+        <remarks>


All of the documentation appears in the Remarks section, with nothing in the Summary section. Should they be moved the the summary?

Done @rpetrusha. Let me know what you think.
I double checked the descriptions in these two sites:
https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-imagedirectoryentrytodata
https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-imagedirectoryentrytodataex

Do you want to add cross references?

rpetrusha · 2019-09-25T19:05:35Z

xml/System.Reflection.PortableExecutable/PEHeader.xml

@@ -44,9 +44,17 @@
        <ReturnType>System.Int32</ReturnType>
      </ReturnValue>


Can we provide a summary? Something like "Represents the header of a Portable Executable (PE) file."?

@tmat does this summary look good to you or would you prefer a different description?

xml/System.Reflection.PortableExecutable/PEHeader.xml

rpetrusha · 2019-09-25T20:17:17Z

xml/System.Reflection.PortableExecutable/PEHeader.xml

+
+## Remarks
+
+Represents `IMAGE_DIRECTORY_ENTRY_TLS`.


Remarks without summary.

rpetrusha

Some additional comments/suggestions.

rpetrusha · 2019-09-25T20:25:50Z

xml/System.Reflection.PortableExecutable/PEHeaderBuilder.xml

@@ -75,8 +75,9 @@
        <param name="sizeOfStackCommit">To be added.</param>


This needs a type summary. Perhaps, "Defines the header for a portable executable (PE) file."

Can we provide descriptions for the constructor's parameters?

@tmat do you agree with the proposed summary?
Sure @rpetrusha I'll complete the constructors.

xml/System.Reflection.PortableExecutable/PEHeaderBuilder.xml

xml/System.Reflection.PortableExecutable/PEHeaders.xml

rpetrusha

Additional changes up to the PEReader.GetSectionData method.

xml/System.Reflection.PortableExecutable/PEReader.xml

rpetrusha · 2019-09-26T20:13:38Z

xml/System.Reflection.PortableExecutable/PEReader.xml

@@ -458,9 +575,13 @@
      </Parameters>
      <Docs>
        <param name="entry">To be added.</param>


Can we provide parameter and return value descriptions here?

rpetrusha · 2019-09-26T20:39:20Z

I've finished reviewing this PR, @carlossanlop. I should have just made the edits directly, but unfortunately I only did that in some cases, and largely left suggestions for the remaining changes.

…into PE

carlossanlop · 2019-09-26T21:47:56Z

Hey @tmat we need your help again.
Can you please provide summaries for these types? There weren't any in the source code. I am not familiar with the purpose of these APIs, so your suggestions would be the most accurate for the documentation.

Here is your PR that introduced them: dotnet/corefx#5354
Here is the roslyn PR with the initial implementation: dotnet/roslyn#7683
Here is the roslyn issue originally tracking the work item: dotnet/roslyn#6904

carlossanlop · 2019-09-26T21:48:51Z

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml

@@ -17,7 +17,7 @@
  </Base>
  <Interfaces />
  <Docs>
-    <summary>To be added.</summary>
+    <summary>Builds PE directories.</summary>


@tmat I added this but I don't think it's descriptive enough. Can you please provide a better summary?

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml

Co-Authored-By: Ron Petrusha <[email protected]>

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml

…into PE

sdmaclea

I am wondering wheether all the "Represents IMAGE_DIRECTORY_ENTRY_*" remarks should be links to some specification. These remarks do not seem to have sufficient context, but perhaps a PE/COFF expert would immediately understand them.

sdmaclea · 2019-09-30T21:52:08Z

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml

@@ -86,7 +86,15 @@
      <Docs>
        <summary>To be added.</summary>
        <value>To be added.</value>
-        <remarks>To be added.</remarks>
+        <remarks>


Do you want to add cross references?

rpetrusha · 2019-10-11T17:14:05Z

@carlossanlop, do you want to address the comments from @sdmaclea, or is this ready to merge?

carlossanlop · 2019-10-11T19:17:35Z

@rpetrusha, let's get this merged. We can add cross references later.

rpetrusha · 2019-10-11T21:40:50Z

Sounds good, @carlossanlop. I'll merge now.

Automatically port Reflection.PortableExecutable.PE* APIs

84b805c

carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases labels Sep 16, 2019

carlossanlop added this to the September 2019 milestone Sep 16, 2019

carlossanlop requested review from GrabYourPitchforks, steveharter, rpetrusha and mairaw September 16, 2019 19:00

carlossanlop self-assigned this Sep 16, 2019

remove top line

239c0d0

rpetrusha suggested changes Sep 25, 2019

View reviewed changes

Update PEMemoryBlock.xml

bec32b5

rpetrusha reviewed Sep 25, 2019

View reviewed changes

rpetrusha reviewed Sep 26, 2019

View reviewed changes

Ron Petrusha added 2 commits September 26, 2019 13:27

In-place edits.

7fa6d07

Edited PEStreamOptions directly

573ab80

rpetrusha reviewed Sep 26, 2019

View reviewed changes

Ron Petrusha and others added 3 commits September 26, 2019 14:02

Revised property descriptions.

7421107

Add summary and value.

8d302cb

Merge branch 'PE' of https://github.com/carlossanlop/dotnet-api-docs …

209ad08

…into PE

carlossanlop requested review from tmat and nguerrera September 26, 2019 21:48

carlossanlop commented Sep 26, 2019

View reviewed changes

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml Outdated Show resolved Hide resolved

carlossanlop and others added 3 commits September 26, 2019 14:52

suggestions by rpetrusha

579a6e3

Co-Authored-By: Ron Petrusha <[email protected]>

suggestions by rpetrusha

8151ae4

Co-Authored-By: Ron Petrusha <[email protected]>

PEReader add missing elements

9b1ce00

carlossanlop commented Sep 26, 2019

View reviewed changes

xml/System.Reflection.PortableExecutable/PEDirectoriesBuilder.xml Outdated Show resolved Hide resolved

carlossanlop and others added 4 commits September 26, 2019 15:17

entry point address

e8fb5b1

more changes

5d027f0

Merge branch 'PE' of https://github.com/carlossanlop/dotnet-api-docs …

0f07006

…into PE

PEHeaderBuilder changes

a437b90

sdmaclea reviewed Sep 30, 2019

View reviewed changes

Ron Petrusha added 3 commits October 2, 2019 13:19

Fixed para tag

94a9ea4

Update PEReader.xml

5f96cc3

Fixed bad crefs

456af06

rpetrusha approved these changes Oct 11, 2019

View reviewed changes

rpetrusha merged commit 9d12dfe into dotnet:master Oct 11, 2019

carlossanlop deleted the PE branch November 6, 2019 18:55

mairaw removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Nov 20, 2019

		@@ -44,9 +44,17 @@
		<ReturnType>System.Int32</ReturnType>
		</ReturnValue>

		@@ -75,8 +75,9 @@
		<param name="sizeOfStackCommit">To be added.</param>

Automatically port Reflection.PortableExecutable.PE* APIs #3193

Automatically port Reflection.PortableExecutable.PE* APIs #3193

Uh oh!

Conversation

carlossanlop commented Sep 16, 2019

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpetrusha commented Sep 26, 2019

Uh oh!

carlossanlop commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sdmaclea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpetrusha commented Oct 11, 2019

Uh oh!

carlossanlop commented Oct 11, 2019

Uh oh!

rpetrusha commented Oct 11, 2019

Uh oh!

Uh oh!