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

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added a comment.

In D67793#1676592 <https://reviews.llvm.org/D67793#1676592>, @labath wrote:

> 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.


That was my initial thought as well, to use a shared_ptr here, and not have any sharing at the lldb_private::File level.     However, I found I couldn't do it that way.   lldb_private::File doesn't really have the semantics of a file object, it has the semantics of a //variable//, which can point to a file object.    Users of lldb_private::File expect to be able to Close() a file and re-open it as something else.   They expect to be able to embed File immediately in other objects.    They expect to be able to make an empty File and check it with IsValid().     I thought it would be best to let them keep using File in that way, so later in the queue I make File copyable and add FileOps class to manage the sharing between copied files.    The other way to do it would be to track down every use of lldb_private::File that isn't through a pointer and convert it to shared_ptr<File> instead.    Then SBFile could just be another shared_ptr<File>.   Should I do it that way?

As far as python copying things goes, that's not a problem.   SWIG will always allocate a SBFile on the heap and copy the pointers to it, it doesn't need an actual C++ copy constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67793