[Lldb-commits] [PATCH] D22629: Rewrite gdb-remote's SendContinuePacketAndWaitForResponse
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Jul 28 03:23:05 PDT 2016
labath added a comment.
In https://reviews.llvm.org/D22629#498135, @clayborg wrote:
> 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.
Agreed. I have a follow-up almost ready. I think I'll be able to upload it as a WIP later in the day. I think it will be easier to explain the idea with a concrete example.
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)
> if you add an assert, please use lldbassert
This comment shows up in the wrong spot, I am moving it to the place where the assert() is
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?
The call is cheap so I don't think it matters, but I've reshuffled it a bit and now it's one.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:297
@@ +296,3 @@
The `assert()` (and two more in the `lock()` function). I am going to change it back to lldbassert, but this checks a very critical part of the internal locking logic, and I would very much doubt our ability recover if these conditions are not met.
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.
In the previous case it does not matter (right now (*)), but it this case it actually saves one string copy: The caller does a substr(1) on the received packet (to remove the leading 'A'), which is a cheap operation on a StringRef, but not on a string.
(*) It does however, future-proof the api. If the caller or the callee end up using something other than a std::string in the future, the API will remain the same, and cheap, as probably StringRef can be used to convert to/from that.
In general, I would reverse the logic here: there is nothing that can be done with a string that you cannot do with a StringRef(**), so why use the former? It's also consistent with the llvm guidelines.
(**) The only exception here is the c_str() function, but if you use StringRefs everywhere then you don't need to call it. Also, mitigating circumstances are: - a lot of our apis already use const char*+length style, which is just a hand-rolled StringRef and is cheap to convert back and forth; by the time you end up calling c_str(), you have probably have already saved a couple of copies, so doing one copy there is not that bad.
More information about the lldb-commits