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

Consider hiding legacy overloads for Math.DivRem #44829

Closed
SingleAccretion opened this issue Nov 17, 2020 · 4 comments
Closed

Consider hiding legacy overloads for Math.DivRem #44829

SingleAccretion opened this issue Nov 17, 2020 · 4 comments
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Numerics

Comments

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Nov 17, 2020

Background and Motivation

#42156 introduced some new overloads for Math.DivRem which follow more modern API design practices (and have way better naming). I think it would make sense to hide the existing ones to avoid potential confusion and save on some Intellisense real estate. The new overloads are functionally the same.

Proposed API

namespace System
{
    public static class Math
    {
+       [EditorBrowsable(EditorBrowsableState.Never)]
        public static int DivRem(int a, int b, out int result);
+       [EditorBrowsable(EditorBrowsableState.Never)]
        public static long DivRem(long a, long b, out long result);
    }
}

Risks

The primary risk would be confusion among people who could be surprised to see the existing overloads disappear. I think it is low though, as the new signatures are quite self-explanatory. Note: this change only impacts new code, so it can be expected that developers working on it would familiarize themselves with the new pattern, especially as it is the only one available for unsigned and native integers.

@SingleAccretion SingleAccretion added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 17, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 17, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

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
Copy link

ghost commented Nov 17, 2020

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

Issue Details
Description:

Background and Motivation

#42156 introduced some new overloads for Math.DivRem which follow more modern API design practices (and have way better naming). I think it would make sense to hide the existing ones to avoid potential confusion and save on some Intellisense real estate. The new overloads are functionally the same.

Proposed API

namespace System
{
    public static class Math
    {
+       [EditorBrowsable(EditorBrowsableState.Never)]
        public static int DivRem(int a, int b, out int result);
+       [EditorBrowsable(EditorBrowsableState.Never)]
        public static long DivRem(long a, long b, out long result);
    }
}

Risks

The primary risk would be confusion among people who could be surprised to see the existing overloads disappear. I think it is quite low though, as the new signatures are quite self-explanatory. Note: this change only impacts new code, so it can expected that developers working on it would familiarize themselves with the new pattern, especially as it is the only one available for unsigned and native integers.

Author: SingleAccretion
Assignees: -
Labels:

api-suggestion, area-System.Numerics, untriaged

Milestone: -

@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 Jan 7, 2021
@tannergooding
Copy link
Member

I've marked this api-ready-for-review so that the design review team can decide if this is appropriate or not.

@terrajobst
Copy link
Member

terrajobst commented Jan 12, 2021

Video

This could confuse people multi-targeting for .NET Standard because neither side is now a superset in IntelliSense. The value of hiding also seems low, especially because developers will typically explore alternative overloads and the tuple overloads will show first.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Numerics
Projects
None yet
Development

No branches or pull requests

5 participants