Introduce StringBuilder utility around raw_string_ostream

[Chandler Carruth]

On Thu, Jun 12, 2014 at 12:54 PM, Chandler Carruth <chandlerc at google.com>
wrote:

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

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