[Lldb-commits] [PATCH] D69488: [LLDB][Python] fix another fflush issue on NetBSD

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

lawrence_danna marked an inline comment as done.
lawrence_danna added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:854
             files = list(i(sbf))
+            # delete them in reverse order, again because each is a borrow
+            # of the previous.
----------------
lawrence_danna wrote:
> mgorny wrote:
> > For the record, this doesn't really guarantee specific order of destruction. Generally, in Python code you shouldn't rely on destructors being called at all and close files manually. That's why `with` is commonly used with files.
> The point is not to rely on the file being closed at a certain time.   They point is that each file added to the list is a borrowed reference to the previous one, and we should not allow those references to become dangling by letting the older ones go to zero-references before the younger ones.
> 
> Now that I say it, I wonder if it was a bad idea to expose this kind of C++ object lifetime semantics to python programs.
> I put in `GetFile` mainly so that in the case that a SBFile was a proxy of a python file, you could get the python file back.
> 
> It might be better to restrict it to that case, and return None instead of a borrowed file in other cases.
> 
> On the other hand, there's value in having a consistent API.  Under the current semantics this just works:
> 
> ```
> print("foo",   file=debugger.GetOutputFile().GetFile())
> ```
> 
> Overall I think I'd lean towards keeping GetFile the way it is, but I can see the argument for restricting it.
On second second thought, there's a better way.   We don't need to expose the borrow semantics, and we don't need to restrict GetFile like that.      We can do the same thing File.cpp does when you ask it for a FILE* on a borrowed FD -- that is, use dup and create one that python can truly own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69488