Introduce StringBuilder utility around raw_string_ostream

[Alp Toker]

On 25/06/2014 23:36, David Blaikie wrote:
> 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.

Good catch, that comment got left over from the earliest revision.

I've made that change, plus a couple more updated uses, and landed this 
in r211749.

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

Yes, I still feel uncomfortable about a type that may or may not own 
storage, but I'm happy to explore that option once someone figures a way 
to code it up.

Thanks for the review

Alp.


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

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