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

[Greg Clayton via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

I am fine with the llvm::StringRef until it causes a crash. So really look at all the ways you construct a llvm::StringRef and make sure it won't assert ever if you leave it as a llvm::StringRef.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:268
@@ +267,3 @@
+            inferior_stdout.append(1, (char)ch);
+    }
+    delegate.HandleAsyncStdout(inferior_stdout);
----------------
Actually, feel free to switch over to using GetHexByteString, but maybe just put the reserve part into that function's implementation? Then this code would be just:

```
packet. GetHexByteString(inferior_stdout);
```

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:31
@@ +30,3 @@
+        virtual void
+        HandleAsyncMisc(llvm::StringRef data) = 0;
+        virtual void
----------------
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????


https://reviews.llvm.org/D22629