labath added a comment.
I think this is looking pretty good overall. I just have a bunch of stylistic
remarks...
================
Comment at:
lldb/packages/Python/lldbsuite/test/tools/lldb-server/registers-target-xml-reading/main.cpp:1
+#include <cstdint>
+
----------------
I don't think the choice of inferior really matters here. This code could just
be like `int main(){}`
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:849
response.PutCString(";qXfer:auxv:read+");
+ response.PutCString(";qXfer:features:read+");
response.PutCString(";qXfer:libraries-svr4:read+");
----------------
You can put this outside the `#ifdef`
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:380-459
+static std::string GetEncodingNameOrEmpty(const RegisterInfo *reg_info) {
+ switch (reg_info->encoding) {
+ case eEncodingUint:
+ return "uint";
+ case eEncodingSint:
+ return "sint";
+ case eEncodingIEEE754:
----------------
These should return a `llvm::StringRef` to avoid copies, and take the argument
as a reference to convey non-nullness.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2786-2910
+ if (object == "features" && annex == "target.xml") {
+ // Make sure we have a valid process.
+ if (!m_debugged_process_up ||
+ (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "No process available");
+ }
----------------
Please move this into a separate function
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2788-2792
+ if (!m_debugged_process_up ||
+ (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID)) {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "No process available");
+ }
----------------
This check could be factored to the front of the function (the svr4 branch does
currently not have it, but it most likely should).
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2841-2843
+ if (!encoding.empty()) {
+ response.Printf("encoding=\"%s\" ", encoding.c_str());
+ }
----------------
We generally don't put curly braces around statements which fit on a single
line.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2871-2894
+ if (reg_info->value_regs &&
+ reg_info->value_regs[0] != LLDB_INVALID_REGNUM) {
+ response.PutCString("value_regnums=\"");
+ int i = 0;
+ for (const uint32_t *reg_num = reg_info->value_regs;
+ *reg_num != LLDB_INVALID_REGNUM; ++reg_num, ++i) {
+ if (i > 0)
----------------
Make a utility function (lambda?) for this?
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
