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

Add Empty property to BinaryData #49670

Closed
ellismg opened this issue Mar 15, 2021 · 10 comments · Fixed by #49777
Closed

Add Empty property to BinaryData #49670

ellismg opened this issue Mar 15, 2021 · 10 comments · Fixed by #49777
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer

Comments

@ellismg
Copy link
Contributor

ellismg commented Mar 15, 2021

Background and Motivation

It would be nice if there was a singleton Empty property for BinaryData, which wraps an empty array. Without this, everyone who wants to return BinaryData needs to create the own static copy of an empty binary data, if they want to share this instance instead of allocating a new empty binary data each time.

Proposed API

Please provide the specific public API signature diff that you are proposing. For example:

namespace System
{
    public class BinaryData {
+        public static BinaryData Empty { get; }
     }
} 

Usage Examples

Before

private static readonly s_EmptyBinaryData = new BinaryData(Array.Empty<byte>());

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's any data to return */) {
        return s_EmptyBinaryData;
    }
}

(or, even worse, since we allocate each time!)

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's any data to return */) {
        return new BinaryData(Array.Empty<byte>());
    }
}

After

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's any data to return */) {
        return BinaryData.Empty;
    }
}

Alternative Designs

We did not consider any alternate designs over a static get only property named Empty.

Risks

None Identified.

@ellismg ellismg added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Data untriaged New issue has not been triaged by the area owner labels Mar 15, 2021
@ghost
Copy link

ghost commented Mar 15, 2021

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

It would be nice if there was a singleton Empty property for BinaryData, which wraps an empty array. Without this, everyone who wants to return BinaryData needs to create the own static copy of an empty binary data, if they want to share this instance instead of allocating a new empty binary data each time.

Proposed API

Please provide the specific public API signature diff that you are proposing. For example:

namespace System
{
    public class BinaryData {
+        public static BinaryData Empty { get; }
     }
} 

Usage Examples

Before

private static readonly s_EmptyBinaryData = new BinaryData(Array.Empty<byte>());

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return s_EmptyBinaryData;
    }
}

(or, even worse, since we allocate each time!)

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return new BinaryData(Array.Empty<byte>());
    }
}

After

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return BinaryData.Empty;
    }
}

Alternative Designs

We did not consider any alternate designs over a static get only property named Empty.

Risks

None Identified.

Author: ellismg
Assignees: -
Labels:

api-suggestion, area-System.Data, untriaged

Milestone: -

@ghost
Copy link

ghost commented Mar 15, 2021

Tagging subscribers to this area: @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

It would be nice if there was a singleton Empty property for BinaryData, which wraps an empty array. Without this, everyone who wants to return BinaryData needs to create the own static copy of an empty binary data, if they want to share this instance instead of allocating a new empty binary data each time.

Proposed API

Please provide the specific public API signature diff that you are proposing. For example:

namespace System
{
    public class BinaryData {
+        public static BinaryData Empty { get; }
     }
} 

Usage Examples

Before

private static readonly s_EmptyBinaryData = new BinaryData(Array.Empty<byte>());

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return s_EmptyBinaryData;
    }
}

(or, even worse, since we allocate each time!)

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return new BinaryData(Array.Empty<byte>());
    }
}

After

public BinaryData SomeMethodThatReturnsData() {
    if ( /* some check that sees if there's an data to return */) {
        return BinaryData.Empty;
    }
}

Alternative Designs

We did not consider any alternate designs over a static get only property named Empty.

Risks

None Identified.

Author: ellismg
Assignees: -
Labels:

api-suggestion, area-System.Memory, untriaged

Milestone: -

@GrabYourPitchforks GrabYourPitchforks 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 Mar 15, 2021
@GrabYourPitchforks
Copy link
Member

Fast-tracking this for review since it's from a partner team.

@GrabYourPitchforks
Copy link
Member

It'd be nice to have an analyzer which detects new T[0], new BinaryData(new T[0]), and similar and can rewrite them all to use the singleton empty APIs.

@terrajobst
Copy link
Member

We already have an analyzer for the first item (#43165 (comment)). Would probably be easy to spot the second as well.

@terrajobst terrajobst added blocking Marks issues that we want to fast track in order to unblock other important work and removed untriaged New issue has not been triaged by the area owner labels Mar 15, 2021
@terrajobst
Copy link
Member

Also marked as blocking so it goes to the front of the queue

@terrajobst terrajobst added code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Mar 15, 2021
@Tornhoof
Copy link
Contributor

May I suggest, if you change the type, to add a bit of documentation to: https://docs.microsoft.com/en-us/dotnet/api/system.binarydata?view=dotnet-plat-ext-5.0 or link https://docs.microsoft.com/en-us/dotnet/api/overview/azure/system.memory.data-readme?

I actually googled for BinaryData, as I was not aware of the type and the second link is hard to find (it shows up for system.memory.data, but not for binarydata), the second link is part of azure for developers for some reason.

@terrajobst terrajobst 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 blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 16, 2021
@terrajobst
Copy link
Member

  • Looks good as proposed
  • We can the analyzer/fixer suggestion from the existing analyzer but it should a get a new ID
namespace System
{
    public class BinaryData
    {
        public static BinaryData Empty { get; }
    }
}

@ellismg
Copy link
Contributor Author

ellismg commented Mar 16, 2021

@terrajobst I assume it would be helpful to the team if I dusted off my fork and submitted a PR, since this is now approved?

@GrabYourPitchforks
Copy link
Member

@ellismg have at it!

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 17, 2021
ellismg added a commit to ellismg/runtime that referenced this issue Mar 17, 2021
This is useful for APIs which return `BinaryData` and want to use a
singleton instead of allocating a new `BinaryData` each
time. Previously, developers would need to create their own static
copy of an empty binary data, now they can just use `Empty`

Fixes dotnet#49670
stephentoub pushed a commit that referenced this issue Mar 18, 2021
This is useful for APIs which return `BinaryData` and want to use a
singleton instead of allocating a new `BinaryData` each
time. Previously, developers would need to create their own static
copy of an empty binary data, now they can just use `Empty`

Fixes #49670
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 17, 2021
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.Memory code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants