Should we implement `pub fn to_string` on Writeable concrete types?

[sffc]

trafficstars

Currently most Writeables implement Display which provides ToString. This means that people can write foo.to_string(), which uses the slower Display-based code path instead of the faster Writeable::write_to_string code path.

We could fix this in many cases by adding a pub fn to_string on concrete types implementing Writeable. This would cause foo.to_string() to use the faster path; the slow path would need a fully qualified call site such as ToString::to_string(&foo).

Note: pub fn to_string would return String, so write_to_string, which returns Cow<str>, would still be optimal. But at least pub fn to_string would be closer to optimal.

Thoughts?

• [x] @Manishearth
• [x] @robertbastian
• [ ] @zbraniecki

[Manishearth]

Seems fine. Should we just return a Cow there too?

[robertbastian]

which uses the slower Display-based code path instead of the faster Writeable::write_to_string code path.

I'm gonna need numbers for this claim.

[sffc]

which uses the slower Display-based code path instead of the faster Writeable::write_to_string code path.

I'm gonna need numbers for this claim.

https://github.com/unicode-org/icu4x/pull/4649

[robertbastian]

Cool

[sffc]

@Manishearth:

1. Is it a breaking change to add a concrete impl of a to_string function on a type that already implements Display?
2. Is it a breaking change to do that if the function returns Cow instead of String?

Seems fine. Should we just return a Cow there too?

Cow is slightly more annoying to deal with; I lean toward returning String directly. If people want the Cow they can call write_to_string as they can now. In most of our impls we return a Cow::Owned anyway.

[Manishearth]

1. Is it a breaking change to add a concrete impl of a to_string function on a type that already implements Display?
2. Is it a breaking change to do that if the function returns Cow instead of String

Best avoided but this kind of thing mostly falls under not being breaking.

[Manishearth]

If it returns Cow i'd perhaps name it something appropriately cowish

[sffc]

OK, consensus on the following proposal then?

1. All ICU4X types that implement Writeable gain their own concrete pub fn to_string(&self) -> String function. The list of types can be found here.
2. Mechanically, this can be done by adding a flag to impl_display_with_writeable! to add the desired function.
3. To be extra safe, make this change in 2.0 (as opposed to 1.5 or 2.1)

LGTM?

[robertbastian]

LGTM with the addition: 4. Properly document this prelude-trait-method shadowing and why we do it (maybe linking to the benchmark)

[Manishearth]

LGTM with 4