Introduce StringBuilder utility around raw_string_ostream

[Alp Toker]

On 27/06/2014 00:48, David Blaikie wrote:
> On Wed, Jun 25, 2014 at 9:53 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Wed, Jun 25, 2014 at 5:52 PM, Alp Toker <alp at nuanti.com> wrote:
>>> On 26/06/2014 03:16, Chandler Carruth wrote:
>>>
>>>> On Thu, Jun 26, 2014 at 2:09 AM, Alp Toker <alp at nuanti.com
>>>> <mailto:alp at nuanti.com>> wrote:
>>>>
>>>>      I've made that change, plus a couple more updated uses, and landed
>>>>      this in r211749.
>> FWIW, perhaps I wasn't clear - but my "I'd like to loop back around
>> with Chandler to nail down the design choice" was "I'm not signing off
>> on this patch without Chandler", so the commit may've been a bit
>> premature.
> Given this confusion, I think appropriate to back this patch out until
> it's actually been signed off on.
>
> Sorry for my vague language/lack of clarity over incremental review
> compared to sign-off.

I don't agree there was any problem with review here. The patch got its 
OKs and looks greats, and even thanks after addressing comments from 
four reviewers on-list and landing.

But I'm going to avoid that discussion and cut to the point, making my 
case for why this patch is fundamentally correct and the alternative 
would be totally flawed:

A critical feature of the patch is guaranteed safety. Where you see a 
"string_ostream" type you get an assurance that the string building 
operations are valid.

Those assurances apply even when the builder is passed by reference. You 
can see the clarity this adds for example in LLVMCreateMessage() and a 
few other places on ToT.

Importantly, there is no locality of construction so *the type becomes 
the documentation*.

If we were to lump storage into raw_string_ostream, this strong safety 
indicated by the type system would disappear along with all the heap 
savings (which simply aren't negligible compared to some of the effort 
going into bit packing elsewhere in the codebase).

If we mix semantics in the base, we'd basically just end up a raw string 
ostream that sometimes isn't raw, even making the name of the type a 
little dubious.

For those reasons I'm unlikely to pursue the flawed alternative 
approach. I don't think it'd be just a small wart -- it would be 
actively detrimental to introduce that in a project that relies on 
building intermediate strings safely to get things done.

Alp.



>
>>>> I don't understand why.
>> Which I guess was Chandler's understanding too (the "why" being "why
>> did you commit?", at least that's how I read it).
>>
>>>> I think my objection to the design direction stands, but I'm tired of
>>>> arguing pointlessly so I'm not going to restate things.
>>>
>>> I actually do care about your suggestion Chandler.
>>>
>>> It's my understanding that, if we get that alternative scheme to work at
>>> some point and agree it's an improvement,
>> I'm not sure why you say this as though there's some unknown about how
>> to make this scheme work. It's pretty simple to just add another ctor
>> to your string_ostream class that takes a reference to a string and
>> uses that just like raw_string_ostream did previously. (& then remove
>> the duplicate class, one way or the other - presumably in favor of the
>> existing class rather than the new one). Yeah, it wastes a little
>> stack when using a user-specified buffer, but that's pretty much
>> unavoidable.
>>
>>>   it'll be entirely compatible with
>>> the important bug fixes and malloc reduction enabled by this commit, which
>>> is where the value lies.
>> The reduction in bug-writability is significant, the malloc reduction
>> I assume is unobservable in any actual performance metric so I'm not
>> really concerned/worried/interested in that side of it (so much so I
>> question the need to proliferate the choice of types here further by
>> providing not just one (where zero might be possible, following
>> Chandler's preferred direction) but two types)
>>
>> - David
>>
>>>
>>> Alp.
>>>
>>> --
>>> http://www.nuanti.com
>>> the browser experts
>>>

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