Introduce StringBuilder utility around raw_string_ostream

[David Blaikie]

Generally looks OK.

The comment "This constructor is specified not to access the string
provided ..." appears to be on the wrong ctor (or perhaps it needs to
be on both ctors?) - the ctor you added (for SmallVector, rather than
std::string) is the one that's used from the derived class.

Though I'm still on the fence about the "one type or two" question
Chandler raised - so wouldn't mind looping back around with him to
nail that down.

The non-proliferation of types is a general good that's worth striving
for, but the "this object could be in one of two states, but the state
is statically determined by the caller and will never change" is
slightly off too. (I tend to twitch at things known statically at a
call site becoming dynamic when a function/type could be split to
preserve that static knowledge - though in this case, since the
functionality's built into the base class anyway, there's no bonus to
preservation of the statically known fact)

On Wed, Jun 25, 2014 at 1:19 PM, Alp Toker <alp at nuanti.com> wrote:
> Updated patch attached. I've fixed a few more invalid string accesses here
> including some that would have generated invalid symbol names in IR.
>
> It wouldn't surprise me if this patch resolves at least some of the disabled
> "flaky" tests like those that generate truncated/unstable output on
> Windows/ARM etc.
>
> I'd really like to land this because it fixes a whole bunch of issues and
> unblocks many more fixes on the clang side.
>
> This is now named "string_ostream" so if someone ever takes the time to
> implement the alternative proposal and it turns out to be genuinely better,
> they can just switch uses over with s/string_ostream/raw_stream_ostream/.
> This patch doesn't preclude that in any way, but does fix real issues today.
>
> David, is this looking good to you with the new updates?
>
> Alp.
>
>
>
>
> On 12/06/2014 23:36, David Blaikie wrote:
>>
>> On Thu, Jun 12, 2014 at 1:25 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> On 12/06/2014 22:37, Chandler Carruth wrote:
>>>>
>>>> On Thu, Jun 12, 2014 at 8:26 PM, Alp Toker <alp at nuanti.com
>>>> <mailto:alp at nuanti.com>> wrote:
>>>>
>>>>
>>>>      On 12/06/2014 21:31, Chandler Carruth wrote:
>>>>
>>>>
>>>>          On Thu, Jun 12, 2014 at 5:21 PM, James Molloy
>>>>          <james at jamesmolloy.co.uk <mailto:james at jamesmolloy.co.uk>
>>>>          <mailto:james at jamesmolloy.co.uk
>>>>
>>>>          <mailto:james at jamesmolloy.co.uk>>> wrote:
>>>>
>>>>              FWIW, I like Alp's suggestion. There are a bunch of ways to
>>>> do
>>>>              string building in LLVM (Twine, raw_string_ostream,
>>>>              std::stringstream). I'd appreciate a single mechanism for
>>>>          making
>>>>              strings.
>>>>
>>>>
>>>>          But that's not what we would get.
>>>>
>>>>
>>>>      I think you're taking an extreme and isolated view on this one
>>>>      Chandler.
>>>>
>>>>
>>>> Dave independently wrote almost the exact same thing.
>>>>
>>>>      What James describes is exactly what we get from the proposed
>>>> patch.
>>>>
>>>>
>>>> We disagree here.
>>>>
>>>>      I've gone ahead and produced examples of invalid string access,
>>>>      and demonstrated how the patch fixes each of these:
>>>>
>>>>
>>>> Yes, two bugs in debugging code. Both of which would also be avoided by
>>>> the simpler suggestion I already made.
>>>
>>>
>>> I don't really agree.
>>>
>>>
>>>> I continue to believe the simpler suggestion could be made to work
>>>> equally
>>>> well with stack based storage.
>>>
>>>
>>> What would happen if every ostream subclass provided integrated storage?
>>> You
>>> put an std::string in raw_string_ostream .. but what do you put in a
>>> raw_fd_ostream?
>>
>> I'm not sure I follow. std::stringstream has internal storage but that
>> doesn't require other things like std::fstream to have internal
>> storage. That doesn't seem like a non-sequitur.
>>
>>> This non sequitur highlights the logical shortcoming of your
>>> simpler suggestion.
>>>
>>> These are streaming interfaces -- the only alternative I see you've
>>> suggested so far is to plug arbitrary storage and provide a weak,
>>> not-really-safe synthesis of what I've proposed,
>>
>> I don't see how either is more or less safe. In one case we have two
>> types, one with internal storage and one without. The other we have
>> one type with the dynamic choice between internal storage and not. In
>> both situations we can still write the same bugs.
>>
>>> adding inappropriate
>>> complexity to raw_string_ostream with no clear benefit.
>>>
>>>
>>>>
>>>>          , and it's going to be quite far from replacing the use cases
>>>>          of Twine.
>>>>
>>>>
>>>>      The latest iteration of the patch is actually a very worthwhile
>>>>      Twine alternative if you're hitting string lifetime issues with
>>>> that.
>>>>
>>>>      Anyway, shouldn't we be working to make the LLVM interfaces safer
>>>>      and more efficient for developers?
>>>>
>>>>
>>>> We are. Part of that is code review. Currently, I don't see any strong
>>>> consensus that your proposal is the right tradeoff, and an alternative
>>>> has
>>>> been proposed in code review that hasn't really been explored.
>>>
>>>
>>> Okay. What evidence do you have for that claim? I'm lacking context.
>>>
>>>
>>> Alp.
>>>
>>>
>>> --
>>> http://www.nuanti.com
>>> the browser experts
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>