labath marked 3 inline comments as done.
labath added inline comments.
================
Comment at: lldb/source/Core/Communication.cpp:318
bool done = false;
+ bool disconnect = false;
while (!done && comm->m_read_thread_enabled) {
----------------
clayborg wrote:
> Can we just init "disconnect" right here?:
>
> ```
> const bool disconnect = comm->GetCloseOnEOF();
> ```
>
That won't work because we there are paths out of this function which should
not disconnect (even in the close-on-eof case). For example, the read thread
can be disabled by setting `comm->m_read_thread_enabled = false` in which case
we should exit even though we have not reached EOF.
================
Comment at: lldb/source/Core/Communication.cpp:322-327
if (bytes_read > 0)
comm->AppendBytesToCache(buf, bytes_read, true, status);
else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
- if (comm->GetCloseOnEOF())
- comm->Disconnect();
+ disconnect = comm->GetCloseOnEOF();
comm->AppendBytesToCache(buf, bytes_read, true, status);
}
----------------
clayborg wrote:
> Is there a reason we call AppendBytesToCache with zero bytes in the else? If
> we do need to call it (it might listen to the "status"?) then this:
>
> ```
> if (bytes_read > 0)
> comm->AppendBytesToCache(buf, bytes_read, true, status);
> else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
> disconnect = comm->GetCloseOnEOF();
> comm->AppendBytesToCache(buf, bytes_read, true, status);
> }
> ```
> can just be:
>
> ```
> comm->AppendBytesToCache(buf, bytes_read, true, status);
> ```
>
> bytes_read is a size_t so it can't be less than zero. Then we can move the
> "disconnect = comm->GetCloseOnEOF();" into the eConnectionStatusEndOfFile
> case in the switch below.
`AppendBytesToCache` calls a callback and I believe the idea is to call it with
a 0-byte buffer on EOF. But the refactor looks good. I've made the
`AppendBytesToCache` call conditional on `bytes_read > 0 || status == EOF` to
preserve the existing behavior of not calling the callback in case of errors.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77295/new/
https://reviews.llvm.org/D77295
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
