[Lldb-commits] [PATCH] D68188: allow arbitrary python streams to be converted to SBFile

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D68188#1689104 <https://reviews.llvm.org/D68188#1689104>, @lawrence_danna wrote:

> In D68188#1687532 <https://reviews.llvm.org/D68188#1687532>, @labath wrote:
>
> > It seems to be that instead of subclassing a fully-functional File class, it would be better to create an abstract class, that both `File` and the new python thingy could implement. That would also create a natural place to write down the FILE* conversion semantics and other stuff we talked about at lldb-dev. I don't have any opinion as to whether the name `File` should denote the new abstract class, or the real thing, but it should most likely be done as a separate patch.
> >
> > What do you think about that?
>
>
> I could do it that way, but I have some reservations about it.    In this version the class hierarchy looks like this:
>
>             File
>              |
>    SimplePythonFile --------+
>     |                       |
>   TextPythonFile     BinaryPythonFile
>   
>
> If we added an abstract base class, I guess it would look like this:
>
>             AbstractFile
>              |        |                
>             File      PythonFile
>              |         |     | |
>            SimplePythonFile  | |
>                              | |
>     + -----------------------+ |  
>     |                          |
>   TextPythonFile     BinaryPythonFile
>   
>
> Does a more complicated class hierarchy like that really solve a problem or make the code easier to understand?


No that wouldn't make things more understandable, but I think it has helped me understand what are you doing here. The usual OO answer to attempts to introduce complex class hierarchies is to try to use object composition instead. Trying to apply that here gives me the following set of classes.

- `File` (or `AbstractFile` ?) - defines just the abstract interface, documents what the interface is
- `NativeFile` (or `File` ?) - implements the File interface via the usual `FILE*` and file descriptor routines (one day these may be split into two classes too)
- `TextPythonFile` - implements the `File` interface via Python `TextIOBase` interface. Does not handle ownership/closing.
- `BinaryPythonFile` - implements the `File` interface via Python `RawIOBase` interface. Does not handle ownership/closing.
- `OwnedPythonFile` - implements the `File` interface by delegating to another `File` object. Has special handling for the close method.

This way the decision whether to "own" something is decoupled from the decision what api do you use to actually write to the file, and I _think_ it would make things clearer. What do you make of that? The thing I don't like about the current solution is that it leaves you with unused `FILE*` members in the Text/BinaryPythonFile classes, and forces the introduction of weird-looking `OverridesIO` methods...

(BTW, as this is C++, it allows you to "cheat" and instead of creating another File object the delegation can be incorporated into the type system by making OwnedPythonFile a template parameterized on the base class. I am fine with both solutions though I think I'd prefer the regular class variant.)



================
Comment at: lldb/include/lldb/API/SBFile.h:16-21
+/* These tags make no difference at the c++ level, but
+ * when the constructors are called from python they control
+ * how python files are converted by SWIG into FileSP */
+struct FileBorrow {};
+struct FileForceScriptingIO {};
+struct FileBorrowAndForceScriptingIO {};
----------------
lawrence_danna wrote:
> labath wrote:
> > I don't think we have anything similar to this in any of our SB classes. It might be better to avoid it. Could the same thing be achieved e.g. with a static factory function (`static SBFile SBFile::Create(FileSP file_sp, bool borrow, bool force_scripting_io)`) ?
> I don't think it can be done that way, because we want those bools to control how the python object is translated into a C++ object, so we need a different swig wrapper for each possibility, which means we need a different c++ function to wrap  for each possibility.
> 
> One way around this would be to have SWIG translate the python object into an intermediate object that just takes a reference, and then perform further translation inside the SB layer.    But we can't do that either, because scripting support is not part of the base LLDB library, it's a plugin.    And in fact Xcode ships two versions of the plugin, one for python2 and one for python3.    The SB needs to be agnostic about different versions of python, so it can't do the translation itself.   
Are you sure about that? Swig allows you to define typemaps for sequences of values. This example is taken from the swig documentation:
```
%typemap(in) (char *str, int len) {
    $1 = PyString_AsString($input);   /* char *str */
    $2 = PyString_Size($input);       /* int len   */
}
```
It seems to me that your typemap could switch on the value of the flag at runtime rather than having compile time switching by swig. This way you wouldn't even need a static function, you could just have an additional constructor with two extra bool arguments.

However, if this is not the preferred syntax for some reason, then we could also have a set of static functions, one for each flag combination, do the conversion that way.

(BTW, the main reason I am trying to avoid the tag structs is because they are wading into uncharted territory in terms of our SB coding guidelines <https://lldb.llvm.org/resources/sbapi.html>)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:500
+    return expected.get();
+  } else {
+    llvm::handleAllErrors(
----------------
no else after return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188