[Lldb-commits] [PATCH] D68737: SBFile::GetFile: convert SBFile back into python native files.

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

lawrence_danna added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:776
     @add_test_categories(['pyapi'])
-    @expectedFailure # FIXME implement SBFile::GetFile
     @skipIf(py_version=['<', (3,)])
----------------
labath wrote:
> lawrence_danna wrote:
> > labath wrote:
> > > So, if I understand correctly, these tests check that you can feed a file opened by python into lldb, and then pump it back out to python. Are there any tests which do the opposite (have lldb open a file, move it to python and then reinject it into lldb)?
> > Yea, `foo is bar` in python is essentially a pointer comparison between the `PyObject*` pointers, so this is testing that you get the same identical file object back in the situations where you should.
> > 
> > There's no test going the other way, because going the other way isn't implemented.   I didn't write anything that could stash an arbitrary `lldb_private::File` in a python object .   If you convert a non-python `File`, you will get a new python file based on the descriptor, if it's available, or the conversion will fail if one is not.   We do test that here, on line 801, we're testing that a `NativeFile` is converted to a working python file and the file descriptors match.
> > 
> > We could, in a follow-on patch make the other direction work with identity too, but right now I can't think of what it would be useful for.
> Right, sorry, that came out a bit wrong. While I think it would be cool to have the other direction be an "identity" too, I don't think that is really necessary. Nevertheless, taking a File out of lldb and then back in will do _something_ right now, and that "something" could probably use a test. I suppose you could construct an SBFile from a path, convert it to a python file and back, and then ensure that reading/writing on those two SBFiles does something reasonable.
I was gonna say that this test already covers it, but then I thought, "oh whatever I'll just write another test".      And the test I wrote uncovered a bug 😐.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68737