clayborg added a comment.
In D106192#2893862 <https://reviews.llvm.org/D106192#2893862>, @OmarEmaraDev
wrote:
> I am currently breaking this patch into smaller independent viable patches as
> suggested.
That will make reviews much easier. Are you going to land the other smaller
diffs first and then update this one after hey have landed?
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+ FileSpec file_spec(m_content);
+ FileSystem::Instance().Resolve(file_spec);
+ return file_spec;
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > 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.
> >
> >
> I was trying to add properties that indicates if the file is local or not and
> if it should be resolved or not. But they all seems identical to the
> need_to_exist property. If m_need_to_exist is false, then the file will not
> be resolved and will not be checked with the host file system (Which is what
> we want to remote files). If it is true, then it will be resolved and checked
> with the host file system (Which is what we want for local files). So adding
> more properties might be redundant? What do you think?
That sounds fine. No need to add more fields that don't make sense
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
bool m_need_to_exist;
+ bool m_required;
};
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > 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.
> The fields that can have a required property is the TextField and its
> derivatives (File, Directory, and Number), so we can can add this logic
> there. This is implemented in D106458.
Sounds good!
================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+ if (!FileSystem::Instance().Exists(file_spec)) {
+ SetError("File doesn't exist!");
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > 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.
> D106459 implements the necessary bits to allow for this.
Sounds good!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106192/new/
https://reviews.llvm.org/D106192
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
