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)
+ continue;
----------------
clayborg wrote:
> 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();
+}
----------------
clayborg wrote:
> 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 @@
+{
+ assert(m_acquired);
+ {
----------------
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
----------------
clayborg wrote:
> 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.
https://reviews.llvm.org/D22629
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
