3

I'm looking for the most straightforward way to convert a byte[] to a struct. My testing indicates that this works:

[StructLayout(LayoutKind.Explicit, Size = OrderStruct.SIZE)]
public unsafe struct OrderStruct
{
    public const int SIZE = 16;

    [FieldOffset(0)]
    private fixed byte _data[OrderStruct.SIZE];

    [FieldOffset(0), MarshalAs(UnmanagedType.I4)]
    public int AssetId;

    [FieldOffset(4), MarshalAs(UnmanagedType.I4)]
    public int OrderQty;

    [FieldOffset(8), MarshalAs(UnmanagedType.R8)]
    public double Price;

    public static OrderStruct FromBytes(ref byte[] data)
    {
        if (data.Length < SIZE)
            throw new ArgumentException("Size is incorrect");

        OrderStruct t = default(OrderStruct);

        fixed (byte* src = data)
        {
            Buffer.MemoryCopy(src, t._data, SIZE, SIZE);
        }

        return t;
    }

    public byte[] ToBytes()
    {
        var result = new byte[SIZE];

        fixed (byte* dst = result)
        fixed (byte* src = this._data)
        {
            Buffer.MemoryCopy(src, dst, result.Length, SIZE);
        }
        return result;
    }
}

Am I missing an edge case or is this an ok way to solve this problem?

Additional info:

  • Performance is important otherwise I would just convert each item individually with BitConverter and this solution is observably faster.
  • I don't really need a generic solution as I'm only going to be doing this for 1 or 2 items in my code base.
  • In my case I don't need to worry about endianness as that is already handled elsewhere.
Servy
  • 202,030
  • 26
  • 332
  • 449
Todd H
  • 55
  • 5
  • 2
    If this is working code, you might get a better answer on [codereview.se]. – Cᴏʀʏ May 29 '18 at 15:12
  • 1
    You don't need the `ref` keyword. – xanatos May 29 '18 at 15:15
  • I like what you have; if it works then run with it. There are semantics.. I would rename the question to read ```convert byte[] to struct``` though as casting is considered if the two are of a same type and this could lead to different answers. If you have a type of two interfaces and are currently recognizing it as one then you can also cast it to the other; if you want it to be a different type altogether you convert it. (At least that's my formal understanding; I may be wrong.) – Michael Puckett II May 29 '18 at 15:19
  • @xanatos, good point, thanks – Todd H May 29 '18 at 15:20
  • Technically you are binary serializing the `struct` here. – xanatos May 29 '18 at 15:23
  • @MichaelPuckettII, I don't have enough rep to edit it. But you're right, I should have said `convert` rather than `cast` – Todd H May 29 '18 at 15:26
  • @ToddHansen You don't need any rep to edit your own post, and you can suggest an edit to anyone else's post, it just requires approval, unlike your own posts. – Servy May 29 '18 at 15:42

1 Answers1

0

This will work, without unsafe code (but in truth it is still quite unsafe)... The try... finally... with empty try {} are for protecting against asynchronous exceptions.

public struct OrderStruct
{
    public const int SIZE = 16;

    public int AssetId;
    public int OrderQty;
    public double Price;

    public static OrderStruct FromBytes(byte[] data)
    {
        if (data.Length < SIZE)
            throw new ArgumentException("Size is incorrect");

        GCHandle h = default(GCHandle);

        try
        {
            try
            {

            }
            finally
            {
                h = GCHandle.Alloc(data, GCHandleType.Pinned);
            }

            OrderStruct t = Marshal.PtrToStructure<OrderStruct>(h.AddrOfPinnedObject());
            return t;
        }
        finally
        {
            if (h.IsAllocated)
            {
                h.Free();
            }
        }
    }

    public byte[] ToBytes()
    {
        var result = new byte[SIZE];

        GCHandle h = default(GCHandle);

        try
        {
            try
            {

            }
            finally
            {
                h = GCHandle.Alloc(result, GCHandleType.Pinned);
            }

            Marshal.StructureToPtr(this, h.AddrOfPinnedObject(), false);
            return result;
        }
        finally
        {
            if (h.IsAllocated)
            {
                h.Free();
            }
        }
    }
}
xanatos
  • 109,618
  • 12
  • 197
  • 280
  • Yeah, some my searching on SO had similar ideas to yours. I'm new to unsafe programming in C# and this will be the first time in our code base, but I'm not opposed to it. I guess I leaned away from this solution as it seemed more complicated that just using unsafe code. For example, I never would have thought about the asynchronous exceptions and wouldn't have included those extra try {} catch{}. – Todd H May 29 '18 at 15:33
  • And what's the point of empty try-finally in this specific case? I understand why it's used in general, but shouldn't you just rely on `GCHandle.Alloc` to do it right itself? – Evk May 29 '18 at 16:24
  • I dont agree with the inner ```try catch``` in this example. Would you mind explaining the purpose of it in the answer? – Michael Puckett II May 29 '18 at 16:57
  • @MichaelPuckettII if you don't agree then you can show something that will prove my code is wrong. Gut feeling isn't enough. Otherwise, being it a quite long explanation, only tangential to the question, you should open a new question, titled for example "why put code in a finally with an empty try" (unless there is something similar already) – xanatos May 29 '18 at 17:08
  • Still I'll put some hints: https://stackoverflow.com/q/2186101/613130 , then https://stackoverflow.com/a/1612363/613130 and then some article about the fact that even assignment (the `=` in `var x = y`) is a distinct operation from the calculation of y. You can see it perfectly well [here](https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgBgATIIwG4CwAUGplgHQBKArgHbBgC2ApqQJK2MBOA9gA4DKnAG5gAxowDO+AoWQBmTACZ0AYXQBvQui2Z5yACzoAsgAouAIwBWjEcHTmLASnWbtrgOLKAEgENqMCIzoABboALzoMIwAZt6UEMDGHj5+AQ5Srq4h4Um+/swAghAQXCKmllDoOSmMACoAnjzMAApg1NSMMGkuWgC+hD1AA=), instructions IL_000b and IL_0010. – xanatos May 29 '18 at 17:38
  • Yes, but in your example you're posting Alloc in a Finally and then Free and a Finally. I'm only asking that you explain this becuase it's not making sense. If it's for threading protection, why would you Alloc on an inner try finally just to free on an outer? – Michael Puckett II May 29 '18 at 18:36
  • @MichaelPuckettII And I'm saying that explaining is long. Post a question, or try to comprehend it from the links I gave you. – xanatos May 29 '18 at 21:48
  • I did look at the links and they don't explain what you're doing. I believe you're going above and beyond for no good reason but I won't argue it. If you're convinced then so be it. – Michael Puckett II May 29 '18 at 23:31
  • @MichaelPuckettII The explanation must be "built". Think what will happen if a Thread.Abort happen between the lines IL_000b and IL_0010, think if the array has been pinned (yes) and think if there is a way to unpin it (no, the .h variable hasn't been assigned) – xanatos May 30 '18 at 06:09
  • @MichaelPuckettII Read what I wrote [here](https://stackoverflow.com/a/7780290/613130), for the reason why a `bool Monitor.Enter()` would be useless and instead .NET has a `void Monitor.Enter(ref bool)` and the comment of Lippert confirming it, and do the same consideration about `GCHandle.Alloc()` – xanatos May 30 '18 at 07:59