clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+ FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
----------------
You don't want to return a unique_ptr here, if you do something like:
```
FieldDelegateUP field_up = form->GetField(1);
```
You have now given away the one reference to this field to the local variable
and it will destruct the item when the local goes out of scope.
Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns
all of its fields, so it is ok to hand out a pointer as others should
temporarily use the FieldDelegate. Also we should bounds check "field_index".
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1985
- FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+ FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
ScrollContext context = field->FieldDelegateGetScrollContext();
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2152-2153
+ if (m_selection_type == SelectionType::Field) {
+ FieldDelegateUP &next_field =
+ m_delegate_sp->GetField(m_selection_index);
+ next_field->FieldDelegateSelectFirstElement();
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2159
- FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+ FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
if (!field->FieldDelegateOnLastOrOnlyElement()) {
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2176
+ if (m_selection_type == SelectionType::Field) {
+ FieldDelegateUP &next_field = m_delegate_sp->GetField(m_selection_index);
+ next_field->FieldDelegateSelectFirstElement();
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2208-2209
+ if (m_selection_type == SelectionType::Field) {
+ FieldDelegateUP &previous_field =
+ m_delegate_sp->GetField(m_selection_index);
+ previous_field->FieldDelegateSelectLastElement();
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2215
- FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+ FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
if (!field->FieldDelegateOnFirstOrOnlyElement()) {
----------------
No need to use a std::unique_ptr here, and GetField should return a pointer as
mentioned above.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2232-2233
+ if (m_selection_type == SelectionType::Field) {
+ FieldDelegateUP &previous_field =
+ m_delegate_sp->GetField(m_selection_index);
+ previous_field->FieldDelegateSelectLastElement();
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2274
if (m_selection_type == SelectionType::Field) {
- FieldDelegateSP &field = m_delegate_sp->GetField(m_selection_index);
+ FieldDelegateUP &field = m_delegate_sp->GetField(m_selection_index);
return field->FieldDelegateHandleChar(key);
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2298
+ DetachOrKillProcessFormDelegate(Process *process) : m_process(process) {
+ if (process->GetShouldDetach()) {
+ SetError("There is a running process, detach from it?");
----------------
When we have interaction, we should allow the user to select kill or detach.
The GetShouldDetach is only for when the user doesn't interact. Can we use a
ChoicesField for this with the two options? We can even go a step further and
add three options: "Detach", "Detach (stopped)", and "Kill". This is easy since
lldb_private::Process has this option on detach:
```
Status Process::Detach(bool keep_stopped);
```
When you detach with "keep_stopped" set to true the process will not resume.
This allows another debugger to be attached, or someone to run another tool on
the process in its suspended state.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+ }
+ window.GetParent()->RemoveSubWindow(&window);
+ }
----------------
Does anyone remove the sub window if the user cancels this dialog?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2342-2343
+ AddTextField("Process Name", GetDefaultProcessName().c_str());
+ m_plugin_field =
+ AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+ m_continue_field = AddBooleanField("Continue once attached.", false);
----------------
most people when attaching won't ever specify the plug-in names. I wonder if we
should include an extra boolean field that enables this? Maybe:
```
m_show_plugin_field = AddBooleanField("Show advanced settings.", false);
```
And then show/hide any settings that aren't commonly used like the "Plugin
Name" field?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2389-2395
+ const char *plugin_name;
+ for (int i = 0;
+ (plugin_name = PluginManager::GetProcessPluginNameAtIndex(i)) !=
+ nullptr;
+ i++) {
+ names.push_back(plugin_name);
+ }
----------------
While loop might be easier here?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2418
+ form_window_sp->SetDelegate(window_delegate_sp);
+
+ return true;
----------------
Is the form for the kill/detach done by the time it reaches this code? If so,
what if the user cancels the Kill/Detach dialog? Can we return false here if
the user cancels?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105655/new/
https://reviews.llvm.org/D105655
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
