clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
This provides a clearer implementation of our async code that grew over the
years and was bolted on, thanks for digging into this.
Looks good a few changes, see inlined comments.
I have some reservations about your future changes to add concurrent packets.
We should discuss this at length before you try to make any changes on the
mailing list.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:57
@@ +56,3 @@
+ std::lock_guard<std::mutex> lock(m_mutex);
+ if (m_async_count == 0)
+ continue;
----------------
if you add an assert, please use lldbassert
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:148-150
@@ +147,5 @@
+ Lock lock(*this, true);
+ if (lock.DidInterrupt())
+ m_should_stop = true;
+ return lock.DidInterrupt();
+}
----------------
Do we need to call lock.DidInterrupt() twice?
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:261-268
@@ +260,10 @@
+ std::string inferior_stdout;
+ inferior_stdout.reserve(packet.GetBytesLeft() / 2);
+
+ uint8_t ch;
+ while (packet.GetHexU8Ex(ch))
+ {
+ if (ch != 0)
+ inferior_stdout.append(1, (char)ch);
+ }
+ delegate.HandleAsyncStdout(inferior_stdout);
----------------
We can probably switch over using a new function that was added after this code
was created:
```
size_t
StringExtractor::GetHexByteString (std::string &str);
```
So this could be:
```
inferior_stdout.reserve(packet.GetBytesLeft() / 2);
packet. GetHexByteString(inferior_stdout.reserve);
```
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:28-29
@@ +27,4 @@
+ virtual ~ContinueDelegate();
+ virtual void
+ HandleAsyncStdout(llvm::StringRef out) = 0;
+ virtual void
----------------
Why not just use "const std::string &" here instead of llvm::StringRef? It is
OK to use std::string in internal APIs and llvm::StringRef buys us nothing.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:31
@@ +30,3 @@
+ virtual void
+ HandleAsyncMisc(llvm::StringRef data) = 0;
+ virtual void
----------------
Why not just use "const std::string &" here instead of llvm::StringRef? It is
OK to use std::string in internal APIs and llvm::StringRef buys us nothing.
https://reviews.llvm.org/D22629
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
