[Lldb-commits] [PATCH] D67891: remove File::SetStream(), make new files instead.

[Pavel Labath via Phabricator via lldb-commits]

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