[Lldb-commits] [PATCH] D67793: new api class: SBFile

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is an interesting undertaking. There's a lot of confusion about FILE* in lldb, and it would be great to clean some of that stuff up.

The api seems reasonably straight-forward. The one thing that struck me is that you're using unique_ptrs for the pimpled objects. That would mean the SBFile objects are non-copyable, which is generally not a good thing, as python really likes to copy things. Looking ahead at your patchset, it looks like I am onto something, as you're then trying to implement shared semantics inside lldb_private::File. That doesn't sound like the most fortunate solution to me, as most of the lldb_private::File users don't need that functionality. I am wondering if it wouldn't be better to use a shared_ptr in SBFile, and implement any extra sharing semantics you need at the SB level. Also, this needs a bunch of tests.

>From a higher level perspective, it would be good to have a couple of paragraphs saying what exactly is the end goal here (what new apis will you introduce, how will they work, etc.). I can sort of guess some of that from looking at the patchset, but it would be nice to have that written down, so that we know we're on the same page. This looks like it is a sufficiently big of a change that it would deserve a quick RFC email to lldb-dev.



================
Comment at: lldb/source/API/SBFile.cpp:20-26
+void SBFile::SetStream(FILE *file, bool transfer_ownership) {
+    m_opaque_up = std::make_unique<File>(file, transfer_ownership);
+}
+
+void SBFile::SetDescriptor(int fd, bool transfer_owndership) {
+    m_opaque_up = std::make_unique<File>(fd, transfer_owndership);
+}
----------------
I think it would be better if these were constructors instead of member functions. That way you might be able to get rid of the all the `if (!m_opaque_up) {` checks as the File member will always be initialized.


================
Comment at: lldb/source/API/SBFile.cpp:21
+void SBFile::SetStream(FILE *file, bool transfer_ownership) {
+    m_opaque_up = std::make_unique<File>(file, transfer_ownership);
+}
----------------
It looks like you need to reformat this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67793