This is an archive of the discontinued LLVM Phabricator instance.

IOHandler: fall back on File::Read if a FILE* isn't available.
ClosedPublic

Authored by lawrence_danna on Oct 7 2019, 6:28 PM.

Details

Summary

IOHandler needs to read lines of input from a lldb::File.
The way it currently does this using, FILE*, which is something
we want to avoid now. I'd prefer to just replace the FILE* code
with calls to File::Read, but it contains an awkward and
delicate workaround specific to ctrl-C handling on windows, and
it's not clear if or how that workaround would translate to
lldb::File.

So in this patch, we use use the FILE* if it's available, and only
fall back on File::Read if that's the only option.

I think this is a reasonable approach here for two reasons. First
is that interactive terminal support is the one area where FILE*
can't be avoided. We need them for libedit and curses anyway,
and using them here as well is consistent with that pattern.

The second reason is that the comments express a hope that the
underlying windows bug that's being worked around will be fixed one
day, so hopefully when that happens, that whole path can be deleted.

Event Timeline

lawrence_danna created this revision.Oct 7 2019, 6:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:28 PM
labath added a comment.Oct 8 2019, 6:02 AM

Even if we ignore the windows parts, there's still a lot of duplication here. The bits that extract a "complete" line from the buffer look like they could be factored into a separate function (Optional<string> GetCompleteLineFromCache(...) ?). It would not hurt to split the parts that read from a FILE and the parts reading from the lldb_private::File into separate functions too...
Other than that, the functionality seems fine to me...

factor out string splitting stuff

labath added inline comments.Oct 9 2019, 6:38 AM
lldb/source/Core/IOHandler.cpp
317–321

line = StringRef(line_buffer.begin(), pos).rtrim("\n\r") ?

326–341

This looks like too much optimization. Why not just append the new stuff and then call SplitLine ?

346–347

Technically, this check is subsumed by the next one.

348

llvm::all_of(line_buffer, ::isspace)

lawrence_danna marked 4 inline comments as done.

review fixes

labath accepted this revision.Oct 11 2019, 1:07 AM
labath added inline comments.
lldb/source/Core/IOHandler.cpp
308–310

Please move these to the beginning of the file, next to all the "using namespace" stuff.

This revision is now accepted and ready to land.Oct 11 2019, 1:07 AM

move using declarations

lawrence_danna marked an inline comment as done.Oct 11 2019, 10:33 AM
This revision was automatically updated to reflect the committed changes.