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

[Pavel Labath via Phabricator via lldb-commits]

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