labath added a comment.
Yeah, after writing the previous comment, I realized that you'll probably want
to subclass File to implement the python stuff (which I agree is a good idea).
So, going for unique_ptr<File> is probably the best way forward here.
Nonetheless, I still believe the return type of the FileSystem method should be
`Expected<unique_ptr<File>>`, as that is the direction both llvm and lldb is
trying to move in (with llvm being much more successful at that, but lldb is
making progress too). If it turns out that a lot of the callers are unable to
do anything useful with the returned error (with llvm::Expected, you are forced
to handle it somehow), we can make a special overload like `OpenIngoringErrors`
which avoids the boilerplate needed to explicitly ignore an error (but I hope
that won't be needed).
The other thing is that the interface changes in the Debugger class are causing
a lot of churn, which makes it hard to figure out what exactly is changing.
Would it be possible to split these out into a separate patch, so that the
functional changes are more obvious here?
================
Comment at: lldb/source/Host/common/FileSystem.cpp:439-440
if (!File::DescriptorIsValid(descriptor)) {
- File.SetDescriptor(-1, options, false);
+ errno = EINVAL;
error.SetErrorToErrno();
} else {
----------------
once this returns `Expected<unique_ptr<File>>`, you can do a `return
llvm::errorCodeToError(std::error_code(errno, std::generic_category()))` here.
================
Comment at: lldb/source/Host/common/FileSystem.cpp:444-445
}
+ if (fileup && !fileup->IsValid())
+ fileup.reset();
return error;
----------------
I don't think this is really needed as the `else` branch is guaranteed to
return a valid `File` object, right? (i.e., you could just do `return
std::make_unique(...)` there...)
================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2319
std::string cmd_file_path = cmd_file.GetPath();
- Status error = FileSystem::Instance().Open(input_file_sp->GetFile(),
cmd_file,
+ FileUP input_file_p;
+ Status error = FileSystem::Instance().Open(input_file_p, cmd_file,
----------------
the lldb convention would be to name these `xxx_up`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67891/new/
https://reviews.llvm.org/D67891
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
