Skip to content

Add support enum and char types in Format-Hex cmdlet #8191

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
iSazonov merged 3 commits into from
Jan 16, 2019
Merged

Add support enum and char types in Format-Hex cmdlet #8191

iSazonov merged 3 commits into from
Jan 16, 2019

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Nov 6, 2018 •
edited
Loading

PR Summary

Fix #3358

Now char and enum types (and arrays of these types) is supported.

PR Checklist

@iSazonov iSazonov self-assigned this Nov 6, 2018
@iSazonov iSazonov requested a review from SteveL-MSFT November 6, 2018 12:10
@iSazonov iSazonov mentioned this pull request Nov 6, 2018
Closed
8 tasks
lzybkr
lzybkr reviewed Nov 16, 2018
View reviewed changes
...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated
Copy link
Contributor

@lzybkr lzybkr Nov 16, 2018

Choose a reason for hiding this comment

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

bool is 1 byte, not 4.

There is a difference between sizeof and Marshal.SizeOf, see See the sizeof documentation, also this blog.

BitConverter.GetBytes(bool) works on managed memory and returns a 1 byte array.

I do think this code should use the managed representation for primitive types.

If Format-Hex is enhanced to support an arbitrary ValueType, then we should probably have a discussion about which representation is more useful because in some cases, the unmanaged layout might be more useful.

Copy link
Collaborator Author

@iSazonov iSazonov Nov 16, 2018

Choose a reason for hiding this comment

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

We could add -Unmanaged switch and by default do managed output.

Copy link
Contributor

@lzybkr lzybkr Nov 16, 2018

Choose a reason for hiding this comment

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

Possibly, but we could also check the attributes on the struct, e.g. most structs used for interop specify [StructLayout(LayoutKind.Sequential)].

Copy link
Collaborator Author

@iSazonov iSazonov Nov 16, 2018

Choose a reason for hiding this comment

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

It looks unnecessarily complicated, especially since CoreCLR/CoreFX uses some tricks to align structures. Seems this attribute does not always mean interop.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jan 9, 2019

Choose a reason for hiding this comment

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

So we agree that this should be 1 byte by default?

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 16, 2018
@iSazonov iSazonov force-pushed the format-hex-primitive branch from 0db3b4e to badc8b1 Compare November 21, 2018 06:48
@iSazonov iSazonov force-pushed the format-hex-primitive branch from badc8b1 to 8608802 Compare December 7, 2018 09:21
@iSazonov iSazonov mentioned this pull request Jan 6, 2019
Closed
@stale
Copy link

stale bot commented Jan 6, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Jan 6, 2019
@iSazonov iSazonov mentioned this pull request Jan 6, 2019
Closed
11 tasks
@iSazonov iSazonov force-pushed the format-hex-primitive branch from 8608802 to c5e7da2 Compare January 6, 2019 12:27
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jan 9, 2019

@SteveL-MSFT @daxian-dbw Could you please review the PR?

@stale stale bot removed the Stale label Jan 9, 2019
@iSazonov iSazonov requested a review from daxian-dbw January 9, 2019 04:54
SteveL-MSFT
SteveL-MSFT requested changes Jan 9, 2019
View reviewed changes
...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jan 9, 2019

Choose a reason for hiding this comment

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

So we agree that this should be 1 byte by default?

@iSazonov iSazonov force-pushed the format-hex-primitive branch from c5e7da2 to 4147dda Compare January 10, 2019 04:35
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jan 10, 2019

Rebased to get latest CI's updates.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Jan 11, 2019

@vexx32 @daxian-dbw Could you please look the PR before we merge?

vexx32
vexx32 reviewed Jan 11, 2019
View reviewed changes
Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Looks pretty solid! Couple comments I would love to clarify before we merge, if we can. 😄

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated
if (baseType.IsArray)
{
baseType = baseType.GetElementType();
dynamic o = inputObject;
Copy link
Collaborator

@vexx32 vexx32 Jan 11, 2019

Choose a reason for hiding this comment

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

Is this used? I don't see this used anywhere after this code point.

Copy link
Collaborator Author

@iSazonov iSazonov Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In next line. Name is very short and not readable.
Renamed.

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
isBool = true;
}

var elementSize = Marshal.SizeOf(baseType);
Copy link
Collaborator

@vexx32 vexx32 Jan 11, 2019

Choose a reason for hiding this comment

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

I could be wrong, but wasn't one of @lzybkr's comments that this would return the wrong size for boolean values?

Copy link
Collaborator Author

@iSazonov iSazonov Jan 11, 2019

Choose a reason for hiding this comment

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

I think it is ok - only local bool var-s have 4 byte size, rest - 1 byte.

@iSazonov iSazonov merged commit 782ef99 into PowerShell:master Jan 16, 2019
@iSazonov iSazonov deleted the format-hex-primitive branch January 16, 2019 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

+2 more reviewers

@lzybkr lzybkr lzybkr left review comments

@vexx32 vexx32 vexx32 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Format-Hex should support all primitive types (including enums) and arrays of these types - instead of this random assortment.

4 participants

@iSazonov @lzybkr @SteveL-MSFT @vexx32