Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add Int128 and UInt128 BitConverter.GetBytes support #80337

Closed
ctigrisht opened this issue Jan 8, 2023 · 7 comments · Fixed by #93817
Closed

[API Proposal]: Add Int128 and UInt128 BitConverter.GetBytes support #80337

ctigrisht opened this issue Jan 8, 2023 · 7 comments · Fixed by #93817
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics

Comments

@ctigrisht
Copy link

ctigrisht commented Jan 8, 2023

Background and motivation

Get the underlying bytes of 128 bit integers for use in binary serialization and others

API Proposal

EDIT:
Signatures for the methods:

namespace System;

public static partial class BitConverter
{
    public static byte[] GetBytes(Int128 value);
    public static Int128 ToInt128(ReadOnlySpan<byte> value);
    public static Int128 ToInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, Int128 value);

    public static byte[] GetBytes(UInt128 value);
    public static UInt128 ToUInt128(ReadOnlySpan<byte> value);
    public static UInt128 ToUInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, UInt128 value);
}

API Usage

public void Example(Int128 value)
{
    var bytes = BitConverter.GetBytes(value);
    var newValue = BitConverter.ToInt128(bytes, 0);
}

Alternative Designs

No response

Risks

No response

@ctigrisht ctigrisht added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 8, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 8, 2023
@ghost
Copy link

ghost commented Jan 8, 2023

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Get the underlying bytes of 128 bit integers for use in binary serialization and others

API Proposal

// check usage

API Usage

public void Example(Int128 value)
{
    var bytes = BitConverter.GetBytes(value);
    var newValue = BitConverter.ToInt128(bytes, 0);
}

Alternative Designs

No response

Risks

No response

Author: ctigrisht
Assignees: -
Labels:

api-suggestion, area-System.Numerics, untriaged

Milestone: -

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 29, 2023
@tannergooding
Copy link
Member

@ctigrisht can you please add the API proposal containing the full signature of both APIs being asked for here?

Once that's done, I can mark this ready for review.

@ctigrisht
Copy link
Author

ctigrisht commented Sep 2, 2023

@tannergooding

There you go :)
I also added it to the original post

byte[] GetBytes(Int128 value);
Int128 ToInt128(ReadOnlySpan<byte> value);
Int128 ToInt128(byte[] value, int startIndex);

byte[] GetBytes(UInt128 value);
UInt128 ToUInt128(ReadOnlySpan<byte> value);
UInt128 ToUInt128(byte[] value, int startIndex);

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Sep 2, 2023
@tannergooding
Copy link
Member

tannergooding commented Sep 2, 2023

Thanks!

I tweaked it just slightly further to be in the format that API review expects (we require the namespace, class, and full signature to be displayed).

I also ensured that TryWriteBytes was included as that will be required for parity with what is exposed for other types.

namespace System;

public static partial class BitConverter
{
    public static byte[] GetBytes(Int128 value);
    public static Int128 ToInt128(ReadOnlySpan<byte> value);
    public static Int128 ToInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, Int128 value);

    public static byte[] GetBytes(UInt128 value);
    public static UInt128 ToUInt128(ReadOnlySpan<byte> value);
    public static UInt128 ToUInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, UInt128 value);
}

@ctigrisht
Copy link
Author

@tannergooding Thanks! My bad, I've never done this before so I was unsure of the format.

@bartonjs
Copy link
Member

Looks good as proposed.

namespace System;

public static partial class BitConverter
{
    public static byte[] GetBytes(Int128 value);
    public static Int128 ToInt128(ReadOnlySpan<byte> value);
    public static Int128 ToInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, Int128 value);

    public static byte[] GetBytes(UInt128 value);
    public static UInt128 ToUInt128(ReadOnlySpan<byte> value);
    public static UInt128 ToUInt128(byte[] value, int startIndex);
    public static bool TryWriteBytes(Span<byte> destination, UInt128 value);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 12, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 7, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 21, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 22, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Projects
None yet
4 participants