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