Introduce StringBuilder utility around raw_string_ostream

[Alp Toker]

On 12/06/2014 14:33, Chandler Carruth wrote:
>
> On Thu, Jun 12, 2014 at 5:25 AM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     Replaces the pattern:
>
>       std::string buf;
>       raw_string_ostream os(buf);
>       ...
>       os.flush();
>       use(buf);
>
>
> So this pattern should be:
>
>   std::string buf;
>   raw_string_ostream os(buf);
>   ...
>   use(os.str());
>
>     with:
>       StringBuilder os;
>       ...
>       use (os.str());
>
>
> Which is now almost identical to this. What's the advantage?

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.


>
> 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.

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.

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.

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.

But my favourite feature about the new StringBuilder is that it makes 
invalid state impossible, which is what most simple string-building 
callers want.


>
> We could even add a 'make_raw_ostream' or some such so that the 
> pattern can become:
>
>   Foo storage;
>   auto os(make_raw_ostream(storage));
>   ...
>   use(os.str());
>
> Thoughts?

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.

Alp.

-- 
http://www.nuanti.com
the browser experts