Add support enum and char types in Format-Hex cmdlet #8191
Conversation
| } | ||
| else if (isBool) | ||
| { | ||
| // bool is 4 bytes apparently |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could add -Unmanaged switch and by default do managed output.
There was a problem hiding this comment.
Possibly, but we could also check the attributes on the struct, e.g. most structs used for interop specify [StructLayout(LayoutKind.Sequential)].
There was a problem hiding this comment.
It looks unnecessarily complicated, especially since CoreCLR/CoreFX uses some tricks to align structures. Seems this attribute does not always mean interop.
There was a problem hiding this comment.
So we agree that this should be 1 byte by default?
iSazonov
added
the
CL-General
iSazonov
force-pushed
the
format-hex-primitive
branch
from
0db3b4e
to
badc8b1
Compare
iSazonov
force-pushed
the
format-hex-primitive
branch
from
badc8b1
to
8608802
Compare
|
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. |
iSazonov
force-pushed
the
format-hex-primitive
branch
from
8608802
to
c5e7da2
Compare
|
@SteveL-MSFT @daxian-dbw Could you please review the PR? |
| } | ||
| else if (isBool) | ||
| { | ||
| // bool is 4 bytes apparently |
There was a problem hiding this comment.
So we agree that this should be 1 byte by default?
iSazonov
force-pushed
the
format-hex-primitive
branch
from
c5e7da2
to
4147dda
Compare
|
Rebased to get latest CI's updates. |
|
@vexx32 @daxian-dbw Could you please look the PR before we merge? |
| if (baseType.IsArray) | ||
| { | ||
| baseType = baseType.GetElementType(); | ||
| dynamic o = inputObject; |
There was a problem hiding this comment.
Is this used? I don't see this used anywhere after this code point.
There was a problem hiding this comment.
In next line. Name is very short and not readable.
Renamed.
| isBool = true; | ||
| } | ||
|
|
||
| var elementSize = Marshal.SizeOf(baseType); |
There was a problem hiding this comment.
I could be wrong, but wasn't one of @lzybkr's comments that this would return the wrong size for boolean values?
There was a problem hiding this comment.
I think it is ok - only local bool var-s have 4 byte size, rest - 1 byte.
PR Summary
Fix #3358
Now
charandenumtypes (and arrays of these types) is supported.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests