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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
