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

TryFormat with format strings as ReadOnlySpan<char> or string #24171

Closed
stephentoub opened this issue Nov 18, 2017 · 2 comments
Closed

TryFormat with format strings as ReadOnlySpan<char> or string #24171

stephentoub opened this issue Nov 18, 2017 · 2 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@stephentoub
Copy link
Member

We've approved and added a bunch of TryFormat methods for doing the equivalent of ToString, but rendering into a supplied Span<char> instead of allocating a string. Many of these TryFormat methods accept a string format argument, just as does ToString. @justinvp raised the question of whether these should instead be ReadOnlySpan<char> format.

Example scenario:
His scenario was in looking at changing the implementation of string.Format, StringBuilder.AppendFormat, etc.: today they allocate substrings when parsing out formats from holes, e.g. "blah {0:D4} blah {1:[##-##-##]}", in order to supply that format string to the formatting API. We can achieve this internally for these methods; the question is whether we should expose that publicly so that other scenarios can benefit from it as well, if there are other scenarios.

Another scenario:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs#L578
BigInteger creates a format string to use when formatting its _sign value, in order to specify the number of digits. If format supported ReadOnlySpan<char>, we could use stack space instead of allocating a string.

The main downside to this change is it makes the methods a bit harder to use, in particular because there's no implicit cast from string to ReadOnlySpan<char>, so instead of passing in a format like "D4", you need to do "D4".AsReadOnlySpan(). TryFormat methods on the following types would be affected:

BigInteger // TryFormat not yet implemented
Byte
DateTime
DateTimeOffset
Decimal
Double // TryFormat not yet implemented
Guid
Int16
Int32
Int64
SByte
Single // TryFormat not yet implemented
TimeSpan
UInt16
UInt32
UInt64

Options

  1. Leave the design as-is. We could using string format, we can address string.Format/StringBuilder.AppendFormat internally, and rare public uses can allocate strings when needed.
  2. Change the methods to use ReadOnlySpan<char> format instead of string format. Callers use AsReadOnlySpan() when they have a string.
  3. Change the methods to use ReadOnlySpan<char> format instead of string format, and add an implicit cast on string from string to ReadOnlySpan<char>.
  4. Keep the existing string format methods, and add overloads that take ReadOnlySpan<char>.
@terrajobst
Copy link
Member

terrajobst commented Nov 21, 2017

Video

We seem to gravitate to (2) because it doesn't require an implicit conversion, but also doesn't prevent us from adding it in the future.

@stephentoub has filed a separate issue to discuss (3) dotnet/corefx#25413.

@karelz karelz changed the title TryFormat with format strings as ReadOnlySpan<char> or string TryFormat with format strings as ReadOnlySpan<char> or string Nov 21, 2017
@karelz
Copy link
Member

karelz commented Nov 21, 2017

FYI: The API review discussion was recorded - see https://youtu.be/o5JFZtgaJLs?t=1593 (18 min duration)

@stephentoub stephentoub self-assigned this Nov 25, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 19, 2020
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
Projects
None yet
Development

No branches or pull requests

4 participants