labath added a comment.
It looks pretty straight-forward. The two main comments I have are:
- implement rtti to get rid of the `GetPythonObject` method on the base class
- it looks like the implementations of new apis you're adding (GetOptions,
mainly) have to go out of their way to ignore errors, only for their callers to
synthesize new Error objects. It seems like it would be better to just use
Expecteds throughout.
================
Comment at: lldb/include/lldb/Host/File.h:56
static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
+ static const char *GetStreamOpenModeFromOptions(uint32_t options);
----------------
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..
================
Comment at: lldb/include/lldb/Host/File.h:316
+ /// The PyObject* that this File wraps, or NULL.
+ virtual void *GetPythonObject() const;
+
----------------
We've spend a lot of time removing python from the lowest layers, and this
let's it back in through the back door. It would be way better to add support
for llvm-style rtti to this class, so that the python code can `dyn_cast` to a
`PythonFile` when it needs to (re)construct the python object. You can take a
look at CPPLanguageRuntime::classof (and friend) for how to implement this in
a plugin-friendly manner.
================
Comment at: lldb/include/lldb/Host/File.h:328
+ /// OpenOptions flags for this file, or 0 if unknown.
+ virtual uint32_t GetOptions() const;
+
----------------
How about returning Expected<OpenOptions> from here?
================
Comment at: lldb/include/lldb/Host/File.h:330
+
+ const char *GetOpenMode() const {
+ return GetStreamOpenModeFromOptions(GetOptions());
----------------
and Expected<const char*> from here?
================
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,)])
----------------
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)?
================
Comment at: lldb/scripts/interface/SBFile.i:81
+
+ %feature("docstring", "convert this SBFile into a python io.IOBase file
object");
+ FileSP GetFile();
----------------
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)?
================
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());
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68737/new/
https://reviews.llvm.org/D68737
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
