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 StringBuilder(Char) #20848

Closed
iSazonov opened this issue Mar 31, 2017 · 6 comments
Closed

Add StringBuilder(Char) #20848

iSazonov opened this issue Mar 31, 2017 · 6 comments

Comments

@iSazonov
Copy link
Contributor

Rationale

In PowerShell repo we got a report PowerShell/PowerShell#3457 about

var allErrors = new StringBuilder('\n');

It is expected that allErrors initialized with '\n' char.
Really compiler silently convert the char -> ushort -> int and use StringBuilder(int capacity) constructor.
Sample http://ideone.com/1pPHlZ

Currently we already have method Append(char value) so suggestion is to add new constructor StringBuilder(char value).

Proposed API

public sealed partial class StringBuilder : ISerializable 
{ 
        // Proposed constructor
        public StringBuilder(char value);
}
@karelz
Copy link
Member

karelz commented Mar 31, 2017

@iSazonov it makes sense to me. Can you please format it into a formal API proposal? It is just much easier to read and review in larger group. Thanks!

@iSazonov
Copy link
Contributor Author

@karelz Issue was reformatted.

@karelz
Copy link
Member

karelz commented Mar 31, 2017

Thanks! I let area owners to check it. From my perspective it is ready for larger API review.

@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2019

This is a good point. The same problem would also exist for this piece of code:

var allErrors = new StringBuilder(' ', 8);

So I suggest we add these two:

public partial class StringBuilder 
{ 
    public StringBuilder(char value);
    public StringBuilder(char value, int repeatCount);
}

@stephentoub mentioned that the sheer fact of us exposing these APIs might cause more issues, when people multi-target and now end up calling the "wrong" APIs. There is also the meta problem that this is a battle we just can't win; C# has an implicit conversion from char to int, so maybe be should just bite the bullet and accept this as a fact of life.

@danmoseley
Copy link
Member

Ouch, I'm surprised this didn't come up before.

@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2019

Video

So after discussion we think it's not worth it. It's been 20 years and this isn't a widespread enough problem.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants