[Lldb-commits] [PATCH] D23884: Add StructuredData unit tests; move packet processing into delegate.

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4812
@@ +4811,3 @@
+static const std::string &GetStructuredDataPacketPrefix() {
+  static const std::string prefix("JSON-async:");
+  return prefix;
----------------
How about just a global `llvm::StringRef`, or even a `StringRef` at local scope?  Doesn't seem worth using a function local static for this.

================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4817
@@ +4816,3 @@
+static StructuredData::ObjectSP
+ParseStructuredDataPacket(const std::string &packet) {
+  Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
----------------
Change the function parameter to an `llvm::StringRef`, and then you can do the following:

================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4821-4838
@@ +4820,20 @@
+  // Verify this J packet is a JSON-async: packet.
+  const std::string &expected_prefix = GetStructuredDataPacketPrefix();
+  const std::string packet_prefix = packet.substr(0, expected_prefix.length());
+  if (packet_prefix != expected_prefix) {
+    if (log) {
+      log->Printf("GDBRemoteCommmunicationClientBase::%s() "
+                  "received $J packet but was not a "
+                  "StructuredData packet: packet starts with "
+                  "%s",
+                  __FUNCTION__, packet_prefix.c_str());
+    }
+    return StructuredData::ObjectSP();
+  }
+
+  // This is an asynchronous JSON packet, destined for a
+  // StructuredDataPlugin.
+
+  // Parse the content into a StructuredData instance.
+  const char *const encoded_json = packet.c_str() + expected_prefix.length();
+
----------------
This entire block becomes:

```
if (!packet.consume_front("JSON-async:")) {
   // print the log statement
}
auto json_sp = StructuredData::ParseJSON(packet);
```

================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4856
@@ +4855,3 @@
+void ProcessGDBRemote::HandleAsyncStructuredDataPacket(llvm::StringRef data) {
+  auto structured_data_sp = ParseStructuredDataPacket(data);
+  if (structured_data_sp)
----------------
This is doing a string copy since you're going from a `StringRef` to a `std::string`.  Use `StringRef` all the way down.

================
Comment at: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp:38
@@ -36,2 +37,3 @@
   unsigned stop_reply_called = 0;
+  std::vector<std::string> structured_data_packets;
 
----------------
This can be a vector of `StringRefs` as well, unless there's some reason you need to throw away the memory backing the `StringRef`, which it doesn't appear you do.

Also, if you have a rough idea of how many `StringRefs` there's going to be ahead of time, or at least an upper bound, then an `llvm::SmallVector<StringRef>` will be more efficient.

================
Comment at: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp:335
@@ +334,3 @@
+  StreamGDBRemote stream;
+  stream.PutEscapedBytes(json_packet.c_str(), json_packet.length());
+  stream.Flush();
----------------
Would be nice to see `PutEscapedBytes` updated to take a `StringRef`.  Every occurrence of passing `const char * str, int len` should be replaced with `StringRef` as we find occurrences of it.


https://reviews.llvm.org/D23884