clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1921
protected:
std::vector<FieldDelegateSP> m_fields;
std::vector<FormAction> m_actions;
----------------
Do these actually need to be shared pointers? They can probably become
std::unique_ptr<T> objects and the FieldDelegateSP would then become
FieldDelegateUP after defining it as a unique_ptr
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2199
+
HandleCharResult SelectePrevious(int key) {
if (m_selection_type == SelectionType::Action) {
----------------
Spelling issue I missed before
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2300
+ types.push_back(std::string("PID"));
+ m_type_field = AddChoicesField("Attach By", 2, types);
+
----------------
I am guessing that the first item in the list is always selected? Might be nice
to provide a way for AddChoicesField to select a different item as the default
selection with another optional parameter that is the index of the item that is
to be selected. Doesn't have to happen in this diff, just thought of it and
wanted to mention this.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2301
+ m_type_field = AddChoicesField("Attach By", 2, types);
+
+ m_pid_field = AddIntegerField("PID", 0);
----------------
remove space
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2302
+
+ m_pid_field = AddIntegerField("PID", 0);
+
----------------
I would be nice if we can eventually auto complete the PID by using the
platform to get a list of processes. Sometimes we are not attached to a remote
platform, but we are always attached to the host platform and should be able to
get a list of process IDs. A new form could be popped up with all of the
process details and allowing the user to filter processes with a TextField,
then a ChoicesField that contains all valid processes the user could attach to
with the pid, process name and possibly other details in the text for each
choice. This doesn't need to be done now, but it would be a good extra
functionality step to complete later if you get everything done on your list.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2303
+ m_pid_field = AddIntegerField("PID", 0);
+
+ m_name_field = AddTextField("Process Name", "");
----------------
remove space
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2304
+
+ m_name_field = AddTextField("Process Name", "");
+
----------------
If we have a target, it might be nice to auto populate this field with the
basename of the target's main executable.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2305
+ m_name_field = AddTextField("Process Name", "");
+
+ m_plugin_field = AddTextField("Plugin Name", "");
----------------
remove space
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2306
+
+ m_plugin_field = AddTextField("Plugin Name", "");
+
----------------
Might be nice to iterate over all process plug-ins and make this into a
ChoicesField where the first entry is "<default>" and the rest are actual
plug-in names. The names can easily be retrieved using:
```
static const char *GetProcessPluginNameAtIndex(uint32_t idx);
```
from PluginManager.h
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2307
+ m_plugin_field = AddTextField("Plugin Name", "");
+
+ m_continue_field = AddBooleanField("Continue once attached.", false);
----------------
remove space
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2346
+ return;
+
+ if (process->GetShouldDetach()) {
----------------
We might want to pop up a dialog here asking the user if they want to kill or
detach since we have a GUI. In the "process attach" command we confirm with the
user if the terminal is interactive.
A few ways we can do this:
- popup a form asking the user to kill/detach/cancel when the user hits submit
on this form
- pop up the dialog before even showing the this form and asking the user to
kill or detach, then show this form after that has been handled
- not allow the Process->Attach to happen by disabling the menu item when a
process is running.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2367
+ m_debugger, "", "", eLoadDependentsNo, nullptr, new_target_sp);
+
+ target = new_target_sp.get();
----------------
We should select the target in he debugger here so that all GUI commands work
correctly after this call if they ask the debugger for its selected target.
```
m_debugger.GetTargetList().SetSelectedTarget(...);
```
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2420-2421
+
+ if (attach_info.GetContinueOnceAttached())
+ process_sp->Resume();
+
----------------
You shouldn't need to do this since you already called
ProcessAttachInfo::SetContinueOnceAttached(true) on the attach_info. LLDB
should take care of this for you already I think.
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
