GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add support enum and char types in Format-Hex cmdlet by iSazonov · Pull Request #8191 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Nov 6, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

Fix #3358

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

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing
      • Issue filed - Issue link: Format-Hex help topic needs several improvements MicrosoftDocs/PowerShell-Docs#2955
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov Ilya (iSazonov) self-assigned this Nov 6, 2018
@iSazonov
Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) November 6, 2018 12:10
@iSazonov
Ilya (iSazonov) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners November 6, 2018 12:10
@iSazonov Ilya (iSazonov) mentioned this pull request Nov 6, 2018
Format-Hex help topic needs several improvements MicrosoftDocs/PowerShell-Docs#2955
Closed
4 of 8 tasks
lzybkr
Jason Shirk (lzybkr) reviewed Nov 16, 2018
View reviewed changes
Comment thread ...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated

@lzybkr Jason Shirk (lzybkr) Nov 16, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Nov 16, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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)].

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jan 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov Ilya (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
Ilya (iSazonov) force-pushed the format-hex-primitive branch from 0db3b4e to badc8b1 Compare November 21, 2018 06:48
@iSazonov
Ilya (iSazonov) force-pushed the format-hex-primitive branch from badc8b1 to 8608802 Compare December 7, 2018 09:21
@iSazonov Ilya (iSazonov) mentioned this pull request Jan 6, 2019
Format-Hex should support all primitive types (including enums) and arrays of these types - instead of this random assortment. #3358
Closed
@stale

stale Bot commented Jan 6, 2019

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stale stale Bot added the Stale label Jan 6, 2019
@iSazonov Ilya (iSazonov) mentioned this pull request Jan 6, 2019
Format-Hex: Handle all primitive types & better array input handling #8596
Closed
7 of 11 tasks
@iSazonov
Ilya (iSazonov) force-pushed the format-hex-primitive branch from 8608802 to c5e7da2 Compare January 6, 2019 12:27
@iSazonov

Ilya (iSazonov) commented Jan 9, 2019

Copy link
Copy Markdown
Collaborator Author

Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) Could you please review the PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@stale stale Bot removed the Stale label Jan 9, 2019
@iSazonov
Ilya (iSazonov) requested a review from Dongbo Wang (daxian-dbw) January 9, 2019 04:54
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Jan 9, 2019
View reviewed changes
Comment thread ...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jan 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ilya (iSazonov) added 2 commits January 10, 2019 09:28
@iSazonov
Add support enum and char types in Format-Hex cmdlet
135bbc0
@iSazonov
Bool is 1 byte
4147dda
@iSazonov
Ilya (iSazonov) force-pushed the format-hex-primitive branch from c5e7da2 to 4147dda Compare January 10, 2019 04:35
@iSazonov

Ilya (iSazonov) commented Jan 10, 2019

Copy link
Copy Markdown
Collaborator Author

Rebased to get latest CI's updates.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jan 11, 2019
View reviewed changes
@iSazonov

Ilya (iSazonov) commented Jan 11, 2019

Copy link
Copy Markdown
Collaborator Author

Rain Sallow (/u/ta11ow) (@vexx32) Dongbo Wang (@daxian-dbw) Could you please look the PR before we merge?
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32
Rain Sallow (/u/ta11ow) (vexx32) reviewed Jan 11, 2019
View reviewed changes

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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. 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated
if (baseType.IsArray)
{
baseType = baseType.GetElementType();
dynamic o = inputObject;

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Jan 11, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread ...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
isBool = true;
}

var elementSize = Marshal.SizeOf(baseType);

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Jan 11, 2019

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

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 Jason Shirk (@lzybkr)'s comments that this would return the wrong size for boolean values?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 11, 2019

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@iSazonov
Minor style fixes
5d518fb
@iSazonov
Ilya (iSazonov) merged commit 782ef99 into PowerShell:master Jan 16, 2019
@iSazonov
Ilya (iSazonov) deleted the format-hex-primitive branch January 16, 2019 11:58
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov
Add support enum and char types in Format-Hex cmdlet (PowerShell#8191)
efb6dfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
+2 more reviewers
@lzybkr Jason Shirk (lzybkr) lzybkr left review comments
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) vexx32 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@iSazonov @lzybkr @SteveL-MSFT @vexx32
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.