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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

While I am fully in favour of removing the SetXXX methods, and it does seem like some amount of shared_ptrs will be needed at the (though I don't fully understand your vission yet), I don't really like this proliferation of shared_ptrs everywhere (there's way too many of shared_ptrs in lldb already). I'd like it to be possible to use these APIs in the simplest cases without any shared_ptrs involved. For instance, instead of `Status FileSystem::Open(FileSP &, FileSpec, uint32_t, uint32_t, bool)`, we could have `Expected<File> FileSystem::Open(FileSpec, uint32_t, uint32_t, bool)` or `Expected<std::unique_ptr<File>> FileSystem::Open(FileSpec, uint32_t, uint32_t, bool)`. The first option would require making the File class movable (so you could write `file = File(...)` instead of `file.SetXXX(...)`, while the second option would enable fully immutable File objects. Given that we already have `File::Close` (and ergo, a natural representation for the moved-from state), I'd try to go for the first option.

Could we make changes like that first, and later decide how much shared_ptring needs to be done? One can always convert a non-shared pointer into a shared one, but the other direction is more tricky. In the mean time, I'll respond to the lldb-dev thread you started (thanks for doing that!) to get a better idea of what you're trying to do here.



================
Comment at: lldb/include/lldb/Core/Debugger.h:123-131
+  lldb_private::File &GetInputFile() { return *m_input_file_sp; }
+
+  lldb_private::File &GetOutputFile() { return m_output_stream_sp->GetFile(); }
+
+  lldb_private::File &GetErrorFile() { return m_error_stream_sp->GetFile(); }
+
+  lldb_private::StreamFile &GetOutputStream() { return *m_output_stream_sp; }
----------------
The lldb_private qualification is superfluous here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67891