labath added inline comments.
================
Comment at: lldb/include/lldb/Host/File.h:56
static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
+ static const char *GetStreamOpenModeFromOptions(uint32_t options);
----------------
lawrence_danna wrote:
> labath wrote:
> > change the argument type to OpenOptions. For some reason, lldb has
> > historically used integer types for passing enumerations around, but we're
> > now slowly changing that..
> Are you sure that's the right thing to do? `eOpenOptionRead |
> eOpenOptionWrite` has type `unsigned int`, not `OpenOptions`.
Ah, right, that was the reason... Regardless, I do thing that's right -- I'll
continue the discussion on the other patch.
================
Comment at: lldb/include/lldb/Host/File.h:331-335
+ static char ID;
+
+ virtual bool isA(const void *classID) const { return classID == &ID; }
+
+ static bool classof(const File *file) { return file->isA(&ID); }
----------------
Please move this somewhere else -- somewhere near the top or bottom of the
class maybe.. It does not make sense to have it in the middle of the open
options stuff.
================
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,)])
----------------
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.
================
Comment at: lldb/scripts/interface/SBFile.i:81
+
+ %feature("docstring", "convert this SBFile into a python io.IOBase file
object");
+ FileSP GetFile();
----------------
lawrence_danna wrote:
> labath wrote:
> > I am somewhat surprised that there's no ownership handling in this patch
> > (whereas ownership transfer was a fairly big chunk of the conversion in the
> > other direction). Could you maybe explain what are the ownership
> > expectations of the file returned through here (and what happens when we
> > close it for instance)?
> Oh, yea good point. `GetFile` returns a borrowed file, it shouldn't be
> closed. I'll add a comment explaining.
Cool, thanks. This description is very helpful.
================
Comment at: lldb/scripts/interface/SBFile.i:93
+ If there is no underlying python file to unwrap, GetFile will
+ use the file descirptor, if availble to create a new python
+ file object using `open(fd, mode=..., closefd=False)`
----------------
s/descirptor/descriptor
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1553-1554
+
+ auto *simple = llvm::dyn_cast<SimplePythonFile>(&file);
+ if (simple)
+ return Retain<PythonFile>(simple->GetPythonObject());
----------------
if (auto *simple = ...)
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:674-682
+ PythonFile(File &file, const char *mode = nullptr) {
+ auto f = FromFile(file, mode);
+ if (f)
+ *this = std::move(f.get());
+ else {
+ Reset();
+ llvm::consumeError(f.takeError());
----------------
lawrence_danna wrote:
> labath wrote:
> > It looks like there's just one caller of this constructor (the "out"
> > typemap for FILE*). Can we just inline this stuff there and delete this
> > constructor?
> It's also called in some parts of ScriptInterpreterPython that are broken for
> other reasons, and are getting fixed later in my patch queue. I'll just put
> a fixme here and delete it when all the other code that uses it also gets
> deleted, ok?
Sounds good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68737/new/
https://reviews.llvm.org/D68737
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
