Introduce StringBuilder utility around raw_string_ostream

[Chandler Carruth]

On Thu, Jun 12, 2014 at 12:45 PM, Alp Toker <alp at nuanti.com> wrote:

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

I understand what you're going for, I just don't really agree with the
tradeoff you're proposing.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140612/b0b0b717/attachment.html>