[Lldb-commits] [PATCH] D74217: Add target.xml support for qXfer request.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Just a couple of comments to make this code more llvm-y. Without those, we have just moved the place which constructs the temporary std::string. I haven't highlighted all the uses, but overall I'd recommend using the `Format` function or something similar instead of `Printf`, as the latter can be error-prone (because of the need to pass the right PRIx macro to get the integer size right) and/or inefficient (need to do the `.str().c_str()` dance on StringRefs).



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1808
+    response.PutCString("encoding:");
+    response.PutCString(encoding.str().c_str());
+    response.PutChar(';');
----------------
Despite the name, PutCString actually takes a StringRef, so you can just drop the `.str().c_str()` thingy. In fact, I'd consider just writing `response << "encoding:" << encoding << ';';`


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2814
+    if (!encoding.empty())
+      response.Printf("encoding=\"%s\" ", encoding.str().c_str());
+
----------------
Similarly, `response << "encoding='" << encoding << "' "`, or `response.Format("encoding='{0}'", encoding)` would be shorter, and avoid string copying.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2906
+    return BuildTargetXml();
+  }
+
----------------
no braces


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74217/new/

https://reviews.llvm.org/D74217