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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
