Skip to content

Conversation

carlossanlop
Copy link
Contributor

No description provided.

@carlossanlop 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 carlossanlop added this to the September 2019 milestone Sep 16, 2019
@carlossanlop carlossanlop self-assigned this Sep 16, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some suggestions for you to consider, @carlossanlop.

@carlossanlop
Copy link
Contributor Author

@tmat would you mind helping us? Since you introduced this type, we need:

  • A type summary. There isn't one in the source. I tried coming up with a description based on the text in the PR and the proposal, but I would prefer it the type creator gives us the most accurate description 😃
  • Address @rpetrusha 's question about HashAlgorithmName: "is this a "checksum algorithm name" or a "checksum based on the algorithm name"?"

Here's the source: src/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/DebugDirectory/PdbChecksumDebugDirectoryData.cs

Here's the PR that introduced: dotnet/corefx#26976

Here's the proposal (the text is the same as in the PR): https://github.com/dotnet/corefx/issues/26935

@carlossanlop carlossanlop requested a review from tmat September 26, 2019 17:06
@tmat
Copy link
Member

tmat commented Sep 26, 2019

The type represents a PDB Checksum debug directory entry as specified here: https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/specs/PE-COFF.md#pdb-checksum-debug-directory-entry-type-19.

This comment is incorrect. Should be "Checksum of the PDB content.".

@carlossanlop
Copy link
Contributor Author

Thank you, @tmat for your quick help. I updated the PR with your suggestions. Let @rpetrusha and I know if you agree with the changes I made so we can merge this.

Copy link
Contributor

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rpetrusha
Copy link

Is this ready to merge, @carlossanlop?

@carlossanlop
Copy link
Contributor Author

Yes, it's ready to merge, @rpetrusha.

@carlossanlop carlossanlop added verify-build-before-merge and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Oct 2, 2019
@rpetrusha rpetrusha merged commit 60e1cf6 into dotnet:master Oct 2, 2019
@carlossanlop carlossanlop deleted the PdbChecksumDebugDirectoryData branch September 22, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants