Introduce StringBuilder utility around raw_string_ostream

[Alp Toker]

On 27/06/2014 01:42, David Blaikie wrote:
> On Thu, Jun 26, 2014 at 3:37 PM, Alp Toker <alp at nuanti.com> wrote:
>> 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,
> You asked me if it was OK to land. I gave some feedback and said I'd
> like to wait. You interpreted that as sign-off. I did not intend it as
> such. We seem to have failed to communicate and I'm trying to clarify
> what I intended to communicate.
>
> I'm sorry for the miscommunication, but I really didn't mean to give
> you the impression that this was ready to commit. Please revert it.

There's obviously no way to revert this without losing the bug fixes. 
I've backed them out together temporarily in r211814.

Could you state any concerns you have clearly? I've provided a 
point-by-point explanation of why we need another type and why it's not 
correct or helpful to conflate the two classes "just because we can" and 
felt that this was well-understood by now.

Alp.


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

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