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

[Todd Fiala via lldb-commits]

tfiala marked 3 inline comments as done.
tfiala added a comment.

I'll make a few more adjustments here based on Zachary's feedback.


================
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;
----------------
zturner wrote:
> 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.
If llvm::StringRef at global scope does not incur a global constructor, that's fine.  If it does, we are prevented from adding global constructors within our products except on a special-case basis.  This would not pass that mark.

Easy enough for me to try, though.  I'll check.

================
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();
+
----------------
zturner wrote:
> This entire block becomes:
> 
> ```
> if (!packet.consume_front("JSON-async:")) {
>    // print the log statement
> }
> auto json_sp = StructuredData::ParseJSON(packet);
> ```
Yep, that looks great.  Thanks!

================
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;
 
----------------
zturner wrote:
> 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.
It's not clear to me what the lifetime is of the packet message content and how that interplays with a StringRef.  I was under the impression that a StringRef will assume the backing store hangs around.  That sounds like a recipe for potential invalid memory access?  I'll check the code, though.  It may be totally fine based on how the packet content is handled.

================
Comment at: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp:335
@@ +334,3 @@
+  StreamGDBRemote stream;
+  stream.PutEscapedBytes(json_packet.c_str(), json_packet.length());
+  stream.Flush();
----------------
zturner wrote:
> 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.
Yep.  I'd prefer to not make that part of this change, though.


https://reviews.llvm.org/D23884