Introduce StringBuilder utility around raw_string_ostream

[David Blaikie]

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.

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