[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target 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.

Many inline comments with simple fixes! Very close.



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2658
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+    m_executable_field = AddFileField("Executable", "", true, true);
+    m_core_file_field = AddFileField("Core File", "", true, false);
----------------
Lots of "true, true" and "true, false" in the file field parameters. Add comments with parameter names:
```
m_executable_field = AddFileField("Executable", "", *need_to_exist=*/true, /*required=*/true);
```



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2659-2662
+    m_core_file_field = AddFileField("Core File", "", true, false);
+    m_symbol_file_field = AddFileField("Symbol File", "", true, false);
+    m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+    m_remote_file_field = AddFileField("Remote File", "", false, false);
----------------
Ditto for all these: add comment variable names as suggested above.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2663
+    m_remote_file_field = AddFileField("Remote File", "", false, false);
+    m_arch_field = AddArchField("Architecture", "", false);
+    m_platform_field = AddPlatformPluginField(debugger);
----------------
inlined comment for "false" above


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2690
+    load_depentents_options.push_back(
+        std::string("Only if the \"Executable\" is an executable file."));
+    load_depentents_options.push_back(std::string("Yes."));
----------------
I would vote to shorten this string with something like "Executable only" with no trailing '.' character.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2691
+        std::string("Only if the \"Executable\" is an executable file."));
+    load_depentents_options.push_back(std::string("Yes."));
+    load_depentents_options.push_back(std::string("No."));
----------------
remove the '.'


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2692
+    load_depentents_options.push_back(std::string("Yes."));
+    load_depentents_options.push_back(std::string("No."));
+    return load_depentents_options;
----------------
remote the '.'


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2698
+    std::string choice = m_load_dependent_files_field->GetChoiceContent();
+    if (choice == "No.")
+      return eLoadDependentsNo;
----------------
remove the '.'. You might want to create a constexpr string value as a static outside of the functions and use them in GetLoadDependentFilesChoices() and GetLoadDependentFiles(). The static would be:
```
static constexpr const char *kLoadDependentFilesNo = "No";
static constexpr const char *kLoadDependentFilesYes = "Yes";
static constexpr const char *kLoadDependentFilesExecOnly = "Only for executable files";
```
Then use these in the GetLoadDependentFilesChoices() and GetLoadDependentFiles() functions. That way if you change the strings, you only have to change one location.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2700
+      return eLoadDependentsNo;
+    if (choice == "Yes.")
+      return eLoadDependentsYes;
----------------
remove the '.'


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2770-2785
+    PlatformSP platform_sp = target_sp->GetPlatform();
+    if (!platform_sp) {
+      SetError("No platform found for target!");
+      return;
+    }
+
+    FileSpec remote_file_spec = m_remote_file_field->GetFileSpec();
----------------
Remove all of this (getting the platform, putting the file), no need to do anything other than module_sp->SetPlatformFileSpec(...) that you are doing below. This information will be used in Process::Launch(...) to do the right thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192