Skip to content

Automatically port Reflection.PortableExecutable.Machine #3192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 30, 2019
Merged

Automatically port Reflection.PortableExecutable.Machine #3192

merged 3 commits into from
Sep 30, 2019

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
sdmaclea
sdmaclea reviewed Sep 16, 2019
View reviewed changes
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
rpetrusha suggested changes Sep 25, 2019
View reviewed changes
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.

Can we provide a type description, @carlossanlop.

@carlossanlop
Copy link
Contributor Author

@sdmaclea @GrabYourPitchforks @steveharter the Reflection.PortableExecutable.Machine enum was introduced for the first time in the initial commit for .NET Core in 2014, which means it was originally introduced in Framework. The triple slash comments do not describe this enum, but based on the content, does this summary sounds good to you? Please confirm so I can merge the change, it's the only modification needed to fully document this type:

<summary>Specifies the target machine's CPU.</summary>

@sdmaclea
Copy link
Contributor

<summary>Specifies the target machine's CPU.</summary>

I would prefer.

<summary>Specifies the target machine's CPU architecture.</summary>

@carlossanlop carlossanlop added changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review verify-build-before-merge and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Sep 25, 2019
sdmaclea
sdmaclea reviewed Sep 25, 2019
View reviewed changes
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

Thanks for the suggestion and review, @sdmaclea, and for the changes, @carlossanlop. I'll merge now.

@rpetrusha rpetrusha merged commit ae289d0 into dotnet:master Sep 30, 2019
@mairaw mairaw removed changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review verify-build-before-merge labels Oct 2, 2019
@carlossanlop carlossanlop deleted the PortableExecutable_Machine branch September 22, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

@steveharter steveharter Awaiting requested review from steveharter

@mairaw mairaw Awaiting requested review from mairaw

2 more reviewers

@sdmaclea sdmaclea sdmaclea left review comments

@rpetrusha rpetrusha rpetrusha approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@carlossanlop carlossanlop

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
Milestone
September 2019
Development

Successfully merging this pull request may close these issues.
4 participants
@carlossanlop @sdmaclea @rpetrusha @mairaw