starred/sharpcompress
1
0
Fork 0
mirror of https://github.com/adamhathcock/sharpcompress.git synced 2026-02-10 13:39:12 +00:00
Code Issues 975 Packages Projects Releases 20 Wiki Activity

Convert Utility.WriteLittleEndian(...) method to be safe #54

New Issue
Open
opened 2026-01-29 22:05:52 +00:00 by claunia · 7 comments
claunia commented 2026-01-29 22:05:52 +00:00
Owner
Copy Link

Originally created by @haykpetros on GitHub (Aug 3, 2015).

It feels like WriteLittleEndian(...) method in Utility.cs file (lines 209 and 251) can be converted to managed code which will make SharpCompress project completely managed and won't require "Allow unsafe code" setting to be set:

public static unsafe void WriteLittleEndian(byte[] array, int pos, short value)
{
    fixed (byte* numRef = &(array[pos]))
    {
        *((short*)numRef) = value;
    }
}
Originally created by @haykpetros on GitHub (Aug 3, 2015). It feels like WriteLittleEndian(...) method in Utility.cs file (lines 209 and 251) can be converted to managed code which will make SharpCompress project completely managed and won't require "Allow unsafe code" setting to be set: ``` cs public static unsafe void WriteLittleEndian(byte[] array, int pos, short value) { fixed (byte* numRef = &(array[pos])) { *((short*)numRef) = value; } } ```
claunia commented 2026-01-29 22:05:53 +00:00
Author
Owner
Copy Link

@adamhathcock commented on GitHub (Aug 3, 2015):

This is just a little performance. I think there is an alternative method for targets that don't like unsafe.

@adamhathcock commented on GitHub (Aug 3, 2015): This is just a little performance. I think there is an alternative method for targets that don't like unsafe.
claunia commented 2026-01-29 22:05:53 +00:00
Author
Owner
Copy Link

@haykpetros commented on GitHub (Aug 5, 2015):

It seems that in my pull request I didn't take into consideration that platform can be big-endian as well, which will make BitConverter.GetBytes() to be wrong when running on such platforms (if indeed WriteLittleEndian() should write bytes using little-endian method). Can some one look into this closer maybe. From what I understand code will need to be modified like following to work properly on all platforms:

public static void WriteLittleEndian(byte[] array, int pos, short value)
{
    byte[] newBytes = BitConverter.GetBytes(value);
    if (!BitConverter.IsLittleEndian)
        Array.Reverse(newBytes);
    Array.Copy(newBytes, 0, array, pos, newBytes.Length);
}

It will most likely not affect any real code, but still better to do it right.

@haykpetros commented on GitHub (Aug 5, 2015): It seems that in my pull request I didn't take into consideration that platform can be big-endian as well, which will make BitConverter.GetBytes() to be wrong when running on such platforms (if indeed WriteLittleEndian() should write bytes using little-endian method). Can some one look into this closer maybe. From what I understand code will need to be modified like following to work properly on all platforms: ``` cs public static void WriteLittleEndian(byte[] array, int pos, short value) { byte[] newBytes = BitConverter.GetBytes(value); if (!BitConverter.IsLittleEndian) Array.Reverse(newBytes); Array.Copy(newBytes, 0, array, pos, newBytes.Length); } ``` It will most likely not affect any real code, but still better to do it right.
claunia commented 2026-01-29 22:05:54 +00:00
Author
Owner
Copy Link

@adamhathcock commented on GitHub (Aug 5, 2015):

I believe the algorithms using that operate in a VM of sorts. Little Endianness is expected regardless of platform.

@adamhathcock commented on GitHub (Aug 5, 2015): I believe the algorithms using that operate in a VM of sorts. Little Endianness is expected regardless of platform.
claunia commented 2026-01-29 22:05:54 +00:00
Author
Owner
Copy Link

@elgonzo commented on GitHub (Sep 7, 2015):

@haykpetros , why make it so overly complicated using BitConverter, if statements, Array.Copy and Array.Reverse? If you want to write a short value (16bit) into a byte array in little-endian with only managed/safe code, just simply do:

array[pos] = (byte) (value & 0xFF); array[pos+1] = (byte) (value >> 8);
@elgonzo commented on GitHub (Sep 7, 2015): @haykpetros , why make it so overly complicated using BitConverter, `if` statements, Array.Copy and Array.Reverse? If you want to write a short value (16bit) into a byte array in little-endian with only managed/safe code, just simply do: ``` array[pos] = (byte) (value & 0xFF); array[pos+1] = (byte) (value >> 8); ```
claunia commented 2026-01-29 22:05:55 +00:00
Author
Owner
Copy Link

@haykpetros commented on GitHub (Sep 8, 2015):

@elgonzo, I personally think it is more readable. There are two overrides to this method, one accepting short another accepting int. Though I don't have personally preference if you write it like in your example using casting, but I am always inclined to make code readable for everyone.

@haykpetros commented on GitHub (Sep 8, 2015): @elgonzo, I personally think it is more readable. There are two overrides to this method, one accepting short another accepting int. Though I don't have personally preference if you write it like in your example using casting, but I am always inclined to make code readable for everyone.
claunia commented 2026-01-29 22:05:55 +00:00
Author
Owner
Copy Link

@weltkante commented on GitHub (Sep 9, 2015):

I don't know how often this method is called, but allocating a new array (via BitConverter.GetBytes) for every number written probably is gonna hurt performance. Considering the point of the original unsafe code was performance, your suggestion sounds like a bad idea.

The suggestion of elgonzo is what one should do if unsafe code isn't allowed.

@weltkante commented on GitHub (Sep 9, 2015): I don't know how often this method is called, but allocating a new array (via `BitConverter.GetBytes`) for every number written probably is gonna hurt performance. Considering the point of the original unsafe code was performance, your suggestion sounds like a bad idea. The suggestion of elgonzo is what one should do if unsafe code isn't allowed.
claunia commented 2026-01-29 22:05:55 +00:00
Author
Owner
Copy Link

@haykpetros commented on GitHub (Sep 9, 2015):

@weltkante well, this particular suggestion wasn't about performance. Originally my suggestion was to remove conditional compilation as it wasn't necessary. I don't have objections to what @elgonzo said, but based on testing on my machine I saw no performance gains.

@haykpetros commented on GitHub (Sep 9, 2015): @weltkante well, this particular suggestion wasn't about performance. Originally my suggestion was to remove conditional compilation as it wasn't necessary. I don't have objections to what @elgonzo said, but based on testing on my machine I saw no performance gains.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
duplicate
enhancement
feedback requested
not bug
pull-request
Mirrored from GitHub Pull Request
 question
up for grabs
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/sharpcompress#54
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?