Method to append any `Display` type to a String

scottmcm · July 24, 2022, 4:24pm

I think the unwrap just isn't that bad for these cases. If it's inlined it might be optimized away, but even if not, it's one obviously-unlikely branch (since the panicking path is #[cold]).

cgwalters · July 24, 2022, 7:57pm

I should have said earlier that performance isn’t my biggest concern here; a bigger problem is that I think seeing a bunch of unwrap calls is usually a “code smell”. Just yesterday I fixed an unwrap in code I wrote (unrelated to Display stuff) to correctly use ? because we were aborting on legitimate regular errors.

scottmcm · July 24, 2022, 8:12pm

I was picturing the unwap being inside a library method, though, just like it is for .to_string():

github.com

rust-lang/rust/blob/2a990f7c0fe4d00042269beb7ee67d0082de4e07/library/alloc/src/string.rs#L2485-L2492


      
          default fn to_string(&self) -> String {
              let mut buf = String::new();
              let mut formatter = core::fmt::Formatter::new(&mut buf);
              // Bypass format_args!() to avoid write_str with zero-length strs
              fmt::Display::fmt(self, &mut formatter)
                  .expect("a Display implementation returned an error unexpectedly");
              buf
          }

That didn't get a special "can't fail Display" version, so I don't think this needs to either.

(Maybe there could be String::push_display, for example.)

Kixunil · August 3, 2022, 9:57am

Some of those APIs could be specialized on owned types to remove extra allocations though, so passing an owned value that's no longer needed may be a good thing. Even in case of push_fmt one could specialize on String to replace self if self.is_empty().

system · November 1, 2022, 9:58am

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.