clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Great start! Many inlined comments.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+ FileSpec file_spec(m_content);
+ FileSystem::Instance().Resolve(file_spec);
+ return file_spec;
----------------
It should be a property of the FileFieldDelegate if the file is for the current
system. We might be specifying a file on a remote system, so it doesn't always
have to exist, nor should it be resolved on the current system. When creating a
target we would always want to specify this file should exist on the current
system.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
bool m_need_to_exist;
+ bool m_required;
};
----------------
Should this to into the FieldDelegate so we don't need to put it into both
FileFieldDelegate and DirectoryFieldDelegate? Then the default
FieldDelegate::FieldDelegateExitCallback() function could be:
```
virtual void FieldDelegate::FieldDelegateExitCallback() {
if (!m_required)
return;
// Check value with another virtual function?
FieldDelegateValueIsValid();
}
```
This seems like many fields would want to require being set and we don't want
to re-invent this for each kind of field.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1346
+ FileSpec file_spec(m_content);
+ FileSystem::Instance().Resolve(file_spec);
+ return file_spec;
----------------
It should be a property of the DirectoryFieldDelegate if the directory is for
the current system. We might be specifying a working directory on a remote
system, so it doesn't always have to exist, nor should it be resolved on the
current system. So if this is for the current system, then it is ok to resolve,
else, we need to just use the path as is.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1912
FileFieldDelegate *AddFileField(const char *label, const char *content,
- bool need_to_exist = true) {
+ bool need_to_exist = true,
+ bool required = true) {
----------------
Should we add a "bool resolve_local_path" so we know if we should resolve the
path on the local system as mentioned above?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1922
const char *content,
- bool need_to_exist = true) {
+ bool need_to_exist = true,
+ bool required = true) {
----------------
Should we add a "bool resolve_local_path" so we know if we should resolve the
path on the local system as mentioned above?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2569
+ TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+ m_file_field = AddFileField("File", "");
+ m_core_file_field = AddFileField("Core File", "", true, false);
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2573
+ m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+ m_arch_field = AddArchField("Architecture", "");
+ std::vector<std::string> load_depentents_options;
----------------
we should add a "Platform" TextField that is populated with all of the platform
names. The default value should be the currently selected platform. To get a
list of all platform plug-in names you can do this like you did for the process
plug-ins:
```
static const char *GetPlatformPluginNameAtIndex(uint32_t idx);
```
To get the current selected platform:
```
ConstString default_platform_name;
lldb::PlatformSP platform_sp = debugger.GetPlatformList()GetSelectedPlatform()
if (platform_sp)
default_platform_name = platform_sp->GetName();
```
This will help with remote targets. The main reason we need this is many linux
or android binaries have very minimal target triples encoded into the ELF
files, so we often need to specify "remote-android" or "remote-linux" when
creating targets. The user can of course set the "Architecture" with a fully
specified triple, but they can also avoid setting the arch and just specify the
platform.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2576
+ load_depentents_options.push_back(
+ std::string("Only if the target is an executable."));
+ load_depentents_options.push_back(std::string("Yes."));
----------------
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+ if (!FileSystem::Instance().Exists(file_spec)) {
+ SetError("File doesn't exist!");
----------------
Won't this be taken care of already by the File field? We should construct the
FileDelegate so that:
- the file must exist
- the file gets resolved on the local system
- the file field is required
This way, the only way for the user to get to the "Create" button if is if they
already were able to set a valid and required by the FileFieldDelegate class.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2614-2618
+ if (!FileSystem::Instance().Open(file_spec,
+ lldb_private::File::eOpenOptionRead)) {
+ SetError("Can't open file!");
+ return nullptr;
+ }
----------------
No need to do this here, we will get an error back from the target creation
below in the "status". So this can be removed. Also, our FileFieldDelegate
should have already shown an error if the file doesn't exist right?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2620
+
+ OptionGroupPlatform platform_options(false);
+ TargetSP target_sp;
----------------
Set the platform name if one was selected and it wasn't the currently selected
one
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2624
+ m_debugger, m_file_field->GetPath(), m_arch_field->GetArchString(),
+ GetLoadDependentFiles(), &platform_options, target_sp);
+
----------------
Maybe add a GetPlatformOptions()?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2637-2639
+ if (!m_symbol_file_field->IsSpecified()) {
+ return;
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2643-2652
+ if (!FileSystem::Instance().Exists(symbol_file_spec)) {
+ SetError("Symbol file doesn't exist!");
+ return;
+ }
+
+ if (!FileSystem::Instance().Open(symbol_file_spec,
+ lldb_private::File::eOpenOptionRead)) {
----------------
Remove, these should have been verified by the FileFieldDelegate already.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2662-2664
+ if (!m_core_file_field->IsSpecified()) {
+ return;
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2668-2677
+ if (!FileSystem::Instance().Exists(core_file_spec)) {
+ SetError("Core file doesn't exist!");
+ return;
+ }
+
+ if (!FileSystem::Instance().Open(core_file_spec,
+ lldb_private::File::eOpenOptionRead)) {
----------------
Remove as FileFieldDelegate should have taken care of this already, or the
process_sp->LoadCore() will return a valid error.
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2686-2688
+ if (!process_sp) {
+ SetError("Unable to find process plug-in for core file!");
+ }
----------------
Remove extra braces on single line if statements (LLVM coding guidelines).
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
