[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected<FileUP>

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

Thanks for splitting this up. Despite the number of comments, I think this is looking pretty good. The main theme of the comments is making sure that we satisfy the contract of the Expected class, which is a bit... unexpected. This is something that every new contributor gets burned by so don't worry about that. I've tried to annotate each place and suggest the possible way to handle it.

Additionally, I think it would be good to remove the ability to swap the file from underneath the stream if it is not too difficult.



================
Comment at: lldb/include/lldb/Core/StreamFile.h:48-53
+  void SetFile(std::shared_ptr<File> file) {
+    if (file)
+      m_file_sp = file;
+    else
+      m_file_sp = std::make_shared<File>();
+  }
----------------
Could we remove this method? It would make things easier to reason about if we could disallow swapping of a File object backing a stream midway through its lifetime. Looking at the existing callers, it does not seem it should be hard to do that -- this is always called immediately after a stream is constructed via patterns like:
```
auto *stream = new StreamFile();
auto file = create_a_file();
if (file.is_ok()) {
  stream->SetFile(file);
  use(stream);
}
```
It should be easy to change that so that the stream is constructed only after we have a valid File object. It would also avoid the need to construct a fake File object just to guarantee it is always initialized.


================
Comment at: lldb/source/API/SBStream.cpp:93
+      FileSystem::Instance().Open(FileSpec(path), open_options);
+  if (file) {
+    StreamFile *newStream = new StreamFile(std::move(file.get()));
----------------
This is the trickiest thing about the Expected class. The idea behind it is it forces you to do something with the error. This means that this code would trigger an assertion if the opening of the file ever fails, as you don't "handle" the error in any way.
Unfortunately, here there is nothing we can do with the error (this is probably a deficiency of this API, but that's also another thing we can't change here), so we need to either explicitly throw it away (`else consumeError(file.takeError())`), or log it `else LLDB_LOG_ERROR(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), file.takeError(), "Cannot open {1}: {0}", path)`


================
Comment at: lldb/source/API/SBStream.cpp:120
   }
-  m_opaque_up.reset(new StreamFile(fh, transfer_fh_ownership));
+  StreamFile *newStream = new StreamFile(fh, transfer_fh_ownership);
 
----------------
why not assign this to the `m_opaque_up` (and avoid having an unowned pointer floating around)?


================
Comment at: lldb/source/API/SBStream.cpp:143
 
-  m_opaque_up.reset(new StreamFile(::fdopen(fd, "w"), transfer_fh_ownership));
-  if (m_opaque_up) {
-    m_is_file = true;
+  StreamFile *newStream = new StreamFile(fd, transfer_fh_ownership);
 
----------------
same here.


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:807-808
       } else {
         result.AppendErrorWithFormat("Failed to open file '%s' for %s.\n",
                                      path.c_str(), append ? "append" : "write");
         result.SetStatus(eReturnStatusFailed);
----------------
Include the error message here, which will also set the "checked" flag inside the expected object.


================
Comment at: lldb/source/Core/StreamFile.cpp:40
+    m_file_sp = std::move(file.get());
+  else
+    m_file_sp = std::make_shared<File>();
----------------
We'll need to ignore/log the error here. Maybe leave a TODO to refactor this so that the caller is able to know whether the file was successfully created or not.


================
Comment at: lldb/source/Core/StreamFile.cpp:49
+    m_file_sp = std::move(file.get());
+  else
+    m_file_sp = std::make_shared<File>();
----------------
same here.


================
Comment at: lldb/source/Expression/REPL.cpp:412
+              file.get()->Close();
+            }
 
----------------
I guess the error can be written to `error_sp` here.


================
Comment at: lldb/source/Host/common/FileCache.cpp:33
+  auto file = FileSystem::Instance().Open(file_spec, flags, mode);
+  if (!file)
     return UINT64_MAX;
----------------
add: `error = std::move(file.takeError());`


================
Comment at: lldb/source/Host/windows/Host.cpp:40
+  if (!imageBinaryP)
+    return false;
+  File &imageBinary = *imageBinaryP.get();
----------------
`return errorToBool(imageBinaryUP.takeError())`


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2326-2329
+    std::string error = llvm::toString(file.takeError());
     result.AppendErrorWithFormat(
         "error: an error occurred read file '%s': %s\n", cmd_file_path.c_str(),
+        error.c_str());
----------------
You could change this into something like `result.AppendErrorWithFormatv("error: an error occurred read file '{0}': {1}\n", cmd_file_path, fmt_consume(file.takeError()))`


================
Comment at: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp:2659
   if (!file) {
     strm.Printf("Error: Failed to open '%s' for writing", path);
     strm.EOL();
----------------
add the actual error to the error message, setting the checked flag


================
Comment at: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp:4595
       } else {
         result.AppendErrorWithFormat("Couldn't open file '%s'", path.c_str());
         result.SetStatus(eReturnStatusFailed);
----------------
add the actual error here


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6279
+          if (!core_file) {
+            error = core_file.takeError();
+          } else {
----------------
this is good :)


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:107
+    ((ProcessGDBRemote *)p)->GetGDBRemote().DumpHistory(stream);
+  }
 }
----------------
ignore the error, I guess?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:958
+  else
+    Reset();
 }
----------------
ignore/log


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:913
+      if (!nullin || !nullout) {
+        result->AppendError("failed to open /dev/null!\n");
+        return false;
----------------
add the error to the error message (you'll probably need to check each variable separately)


================
Comment at: lldb/source/Target/Platform.cpp:1232
+  if (!source_file)
+    return Status("PutFile: unable to open source file");
+  Status error;
----------------
`return Status(source_file.takeError())`


================
Comment at: lldb/unittests/Host/FileSystemTest.cpp:292-322
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
----------------
What's the point of having both of these tests? The error code should be the same no matter how you retrieve it from the expected object, so this is more of a unit test for the Expected class, than anything else...


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:586
+                                          File::eOpenOptionRead);
+  ASSERT_TRUE(!!file);
+  PythonFile py_file(*file.get(), "r");
----------------
`ASSERT_THAT_EXPECTED(file, llvm::Succeeded())`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67996/new/

https://reviews.llvm.org/D67996