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

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

lawrence_danna marked 2 inline comments as done.
lawrence_danna added inline comments.


================
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 {};
----------------
labath wrote:
> 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>)
I really do think it can't be done with swig.   You can group multiple arguments together on the C++ side, but not on the python side.    

I think I did find a reasonable way to deal with it though without uglying up the SB headers with those tags.   I added some statics with %extend, and then wrapped them with a python-friendly classmethod called Create, with both flags as keyword args.    

How's that look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188