Skip to content

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Sep 2, 2025

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 2, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 2, 2025
@@ -637,7 +637,8 @@ internal static void GetBytes(in decimal d, Span<byte> buffer)

internal static decimal ToDecimal(ReadOnlySpan<byte> span)
{
Debug.Assert(span.Length >= 16);
span = span.Slice(0, 16);

int lo = BinaryPrimitives.ReadInt32LittleEndian(span);
int mid = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(4));
int hi = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(8));
Copy link
Member

@EgorBo EgorBo Sep 2, 2025

Choose a reason for hiding this comment

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

should we just re-order it with int flags = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(12));?

Copy link
Contributor Author

@xtqqczze xtqqczze Sep 2, 2025

Choose a reason for hiding this comment

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

I think a reordered version would be less readable. You’d probably need something like:

        int flags = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(12, 4));
        int hi = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(8, 4));
        int mid = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(4, 4));
        int lo = BinaryPrimitives.ReadInt32LittleEndian(span.Slice(0, 4));

Doing the slicing up front (by replacing the Debug.Assert) makes the intent more obvious, avoids repetition, and doesn’t require bending the code to work around JIT limitations. It also gives the most efficient codegen in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants