[Lldb-commits] [PATCH] D82857: [LLDB] Add per-thread register infos shared pointer in gdb-remote

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:34
     ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
-    GDBRemoteDynamicRegisterInfo &reg_info, bool read_all_at_once,
+    GDBRemoteDynamicRegisterInfoSP reg_info, bool read_all_at_once,
     bool write_all_at_once)
----------------
reg_info_sp


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:36
     bool write_all_at_once)
-    : RegisterContext(thread, concrete_frame_idx), m_reg_info(reg_info),
+    : RegisterContext(thread, concrete_frame_idx), m_reg_info_sp(reg_info),
       m_reg_valid(), m_reg_data(), m_read_all_at_once(read_all_at_once),
----------------
nit: std::move(reg_info) (and changing the uses below to m_reg_info_sp).


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h:43
+
+  void CloneFrom(GDBRemoteDynamicRegisterInfoSP process_reginfo);
 };
----------------
This is basically making a copy, right? Could we just use the copy constructor for this (maybe coupled with declaring the class `final` to avoid the possibility of slicing)?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:399-405
+  if (!m_register_info_sp)
+    m_register_info_sp = std::make_shared<GDBRemoteDynamicRegisterInfo>();
+
+  if (!force && m_register_info_sp->GetNumRegisters() > 0)
     return;
 
+  m_register_info_sp->Clear();
----------------
How about:
```
if (!force &&  m_register_info_sp)
  return;
m_register_info_sp = std::make_shared<GDBRemoteDynamicRegisterInfo>();
```


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp:322
 
+void ThreadGDBRemote::SetThreadRegisterInfo() {
+  ProcessSP process_sp(GetProcess());
----------------
Is this going to have more callers in the future? It seems that things would be a lot simpler if we just did this directly in the constructor...


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

https://reviews.llvm.org/D82857