This is an archive of the discontinued LLVM Phabricator instance.

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

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



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

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.

Diff Detail

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

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


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


Technically, this check is subsumed by the next one.


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.

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.