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]: void-returning Encode callback for AsnWriter #112675

Closed
vcsjones opened this issue Feb 19, 2025 · 2 comments · Fixed by #112921
Closed

[API Proposal]: void-returning Encode callback for AsnWriter #112675

vcsjones opened this issue Feb 19, 2025 · 2 comments · Fixed by #112921
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Formats.Asn1 in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Feb 19, 2025

Background and motivation

In #75759 we approved two APIs that use a callback mechanism for encoding when using AsnWriter. Both of the added methods return a TReturn.

When wiring this in through the Libraries, there are several places that look like this:

inner.Encode(writer, static (writer, encoded) =>
{
writer.WriteBitString(encoded);
return (object?)null;
});

They modify some state that gets passed in, but don't need to return anything, which results in a return (object?)null.

We should overload Encode to accept an Action callback and return void.

API Proposal

namespace System.Formats.Asn1;

public partial class AsnWriter {
#if NET9_0_OR_GREATER
    public void Encode<TState>(
        TState state,
        Action<TState, ReadOnlySpan<byte>> encodeCallback) where TState : allows ref struct;
#endif
}

API Usage

From the example above, that would become a nicer-looking:

inner.Encode(writer, static (writer, encoded) =>
{
    writer.WriteBitString(encoded);
});

Alternative Designs

There is, yet another, overload we could add - which is one that accepts no state and returns nothing. I don't know how useful that is. That would imply that global state is being modified, or something captured.

That would look like:

public void Encode(Action<ReadOnlySpan<byte>> encodeCallback);

But I see little utility in it. It can be added for symmetry if that is important.

Risks

No response

@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Formats.Asn1 labels Feb 19, 2025
@vcsjones vcsjones added this to the 10.0.0 milestone Feb 19, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-formats-asn1, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@bartonjs bartonjs 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 labels Feb 19, 2025
@bartonjs
Copy link
Member

bartonjs commented Feb 25, 2025

Video

  • Looks good as proposed.
  • Since the API exists purely for perf, there's no reason to add the Action overload that doesn't take a state.
namespace System.Formats.Asn1;

public partial class AsnWriter {
#if NET9_0_OR_GREATER
    public void Encode<TState>(
        TState state,
        Action<TState, ReadOnlySpan<byte>> encodeCallback) where TState : allows ref struct;
#endif
}

@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 Feb 25, 2025
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Feb 25, 2025
@vcsjones vcsjones self-assigned this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Formats.Asn1 in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants