This is an archive of the discontinued LLVM Phabricator instance.

Remove SBStream accessor that lets you manipulate the internal buffer.
ClosedPublic

Authored by zturner on Nov 15 2016, 1:44 PM.

Details

Summary

Instead of having overloads that return std::string& and const std::string&, instead have one function that returns llvm::StringRef. People should not be modifying the internal contents of a stream's buffer, as that goes against the concept of a stream, which is supposed to provide forward-only access. This makes it easier in the future to port StreamString over to llvm::raw_ostream.

I don't need a detailed review of the code, I mostly just need to make sure it compiles on OSX. I made sure it compiles on Linux and Windows, but I can't easily test OSX.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 78068.Nov 15 2016, 1:44 PM
zturner retitled this revision from to Remove SBStream accessor that lets you manipulate the internal buffer..
zturner updated this object.
zturner added reviewers: beanz, tfiala.
zturner added a subscriber: lldb-commits.
tfiala edited edge metadata.Nov 15 2016, 5:45 PM

I can run this through in the morning on macOS.

Hi Zachary,

I'm uploading a file of diffs I needed to get the macOS build to compile and link. There were a small handful of macOS-only paths that needed an adjustment. You'll want to look those over to see if I did them the right way.

I'm now in the process of running the tests. Back on that in a bit.

TestTerminal.py's test_launch_in_terminal() is timing out consistently with the current change + my diff attached above.

I'm looking into it now. (It quite possibly is due to the build fixes I did above).

I'm attaching the 'sample' (callstack sampling) from the failure here.

tfiala accepted this revision.Nov 16 2016, 11:28 AM
tfiala edited edge metadata.

Here is the adjusted patch that fixes the issue I was seeing on TestTerminal.py. My first set of changes had a lifetime issue where I needed a const char* that was synthesized on the fly and went away by the time I needed it.

LGTM on macOS with this patch applied:

This revision is now accepted and ready to land.Nov 16 2016, 11:28 AM
This revision was automatically updated to reflect the committed changes.