<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 12, 2014 at 12:54 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div><div class="h5"><br><div class="gmail_quote">On Thu, Jun 12, 2014 at 12:45 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="overflow:hidden">On 12/06/2014 14:33, Chandler Carruth wrote:<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Thu, Jun 12, 2014 at 5:25 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
Replaces the pattern:<br>
<br>
std::string buf;<br>
raw_string_ostream os(buf);<br>
...<br>
os.flush();<br>
use(buf);<br>
<br>
<br>
So this pattern should be:<br>
<br>
std::string buf;<br>
raw_string_ostream os(buf);<br>
...<br>
use(os.str());<br>
<br>
with:<br>
StringBuilder os;<br>
...<br>
use (os.str());<br>
<br>
<br>
Which is now almost identical to this. What's the advantage?<br>
</blockquote>
<br></div>
Unfortunately people don't get the pattern right. There's that tempting 'buf' string that ends up getting accessed incorrectly. People tend to flush either too often or not enough, and just the casual replace in the patch found a handful of these.<div>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The reason I like the current form is that it makes it obvious and easy to re-use storage where that is reasonable, and it makes variations with SmallString more obviously the same pattern with a different storage.<br>
</blockquote>
<br></div>
We'd keep the raw_string_ostream class with this scheme, and it has several legitimate users. But this patch deals with the ones that really don't want to know about the string backing.<br>
<br>
By hiding the storage for callers that don't care so much whether it's backed by std::string, we can start to do smart things like experimenting with SmallString<> backing and providing a templatized version. Right now these callers are stuck with the worst-case std::string allocator and this patch breaks away from that.<br>
<br>
And it's also easy to go from there to hooking this up to PGO that determines optimal backing sizes for each occurrence to avoid mallocs. That flexibility wasn't really there before.<br>
<br>
But my favourite feature about the new StringBuilder is that it makes invalid state impossible, which is what most simple string-building callers want.<div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
We could even add a 'make_raw_ostream' or some such so that the pattern can become:<br>
<br>
Foo storage;<br>
auto os(make_raw_ostream(storage));<br>
...<br>
use(os.str());<br>
<br>
Thoughts?<br>
</blockquote>
<br></div>
The dummy storage variable is still there in that last snipped, and I'm proposing to get rid of it. It seems consistently better to hide it so we can tweak it for performance and ensure safe usage -- think of it as RAII for building strings combining storage and interface.</div>
</blockquote></div><br></div></div>I understand what you're going for, I just don't really agree with the tradeoff you're proposing.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I think the current interface is fine, and having the ability to deal with the storage independently of the stream is necessary in some cases. I even think it is good in some cases to expose to the client of the interface that they have storage and should consider its lifetime and semantics. As a consequence, I don't think it is worth adding yet another type and pattern for the minor (IMO) benefit of avoiding one possible misuse. I'd rather just teach people to use the str() method, and remove the misuses of flush when we find them (either through an audit or in code review). Now, if there are simple ways to make the interfaces of the stream classes better so that this is easier to get right and harder to get wrong, I'm all for that.</div>
</div>
</blockquote></div><br></div><div class="gmail_extra">Another point to consider: we already have a much more pure builder style API for constructing strings: Twine. It's API isn't structured in a builder-like form, but its behavior is much more akin to a builder than the ostream behavior is. There is still quite a bit of code that uses a Twine as a builder, just without the common builder syntax.</div>
</div>