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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
