[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I like the new detach/kill dialog as it incorporates many of your features that you added to the forms! Very close, just a few minor issues and this will be good to go.



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegate *GetField(uint32_t field_index) {
+    if (field_index < m_fields.size())
----------------
You are correct unless the caller of the function doesn't use a reference. All of the call sites you had in here were using references, so there wasn't an issue. But if someone else comes along and actually wrote code like the snipped I posted above, it would transfer ownership and that would be bad.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2051-2054
+      if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+        continue;
       bool is_field_selected = a_field_is_selected && m_selection_index == i;
+      FieldDelegate *field = m_delegate_sp->GetField(i);
----------------



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2421
+    WindowDelegateSP window_delegate_sp =
+        WindowDelegateSP(new FormWindowDelegate(form_delegate_sp));
+    form_window_sp->SetDelegate(window_delegate_sp);
----------------
Ok, thanks for the explanation. It makes sense.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2425
+    return true;
+  }
+
----------------
Should be easy to verify if it is needed or not. If it is, we will need to add it back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655