[Lldb-commits] [PATCH] D22629: Rewrite gdb-remote's SendContinuePacketAndWaitForResponse

[Pavel Labath via lldb-commits]

labath added a comment.




================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:269
@@ +268,3 @@
+void
+GDBRemoteClientBase::OnRunPacketSent(bool first)
+{
----------------
Will do. At that point this whole function becomes pretty irrelevant and i will inline it back into the caller.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:31
@@ +30,3 @@
+        virtual void
+        HandleAsyncMisc(llvm::StringRef data) = 0;
+        virtual void
----------------
clayborg wrote:
> I just really don't like that a llvm::StringRef constructed with a null "const char *" will crash LLDB due to an assertion. It has cause crashes in the past when you have the result of a function being fed into a llvm::StringRef. I really want to make a lldb::StringRef that subclasses llvm::StringRef and fixes this as I really don't think it is useful. Why can't an llvm::StringRef be constructed with NULL????
I understand your concerns, but i also see the other side's point, who consider a null pointer an invalid input and the responsibility of the user to deal with that.

That said, we as a user have a lot of places which treat null as an empty string, so we need to do something about that. Instead of a new (sub-)class, I'd go for making a factory function instead (`lldb::makeStringRef()`?) and say we use that whereever we are not sure about the nullness of the pointer. The advantage of that being you don't have to worry about type conversions when dealing with different StringRefs (and once our StringRef is constructed, it works just like the vanilla object). In time, the need for that function should even diminish, as once we are already using a StringRef, we don't have to worry about the whole null pointer business.

BTW, constructing a std::string from a null pointer is also undefined behaviour, and implementations I've seen either assert or crash when trying do to a strlen().


https://reviews.llvm.org/D22629