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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to