[Lldb-commits] [PATCH] D68547: exception handling in PythonDataObjects.

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

lawrence_danna added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:280
+  template <typename... Args>
+  Expected<PythonObject> CallMethod(const char *name, const char *format,
+                                    Args... args) {
----------------
labath wrote:
> Implemented like this, this method still requires one to work with python types directly, and deal with things (signatures) that is better left to the glue code. As this is c++, what do you think of an implementation like:
> ```
> template<typename T, typename Enable = void> struct PythonFormat;
> template<> struct PythonFormat<unsigned long long> {
>     static constexpr char format = 'K';
>     static auto get(unsigned long long value) { return value; }
> };
> template<typename T>
> struct PythonFormat<T,
>         typename std::enable_if<
>             std::is_base_of<PythonObject ,T>::value>::type> {
>     static constexpr char format = 'O';
>     static auto get(const T &value) { return value.get(); }
> };
> // etc.
> 
> template<typename... T>
> Expected<PythonObject> CallMethod(const char *name, const T &... t) {
>     const char format[] = { '(', PythonEncoding<T>::format..., ')'};
>     PyObject *obj = PyObject_CallMethod(m_py_obj, name, format, PythonFormat<T>::get(t)...);
>     ...
> }
> ```
> This should make calling a python method as close to calling a native one as possible. The main downside of that is that it is impossible to use fancier formats like `s#`. If we really wanted to, we could make that work too (at the expense of a fairly large increase in template complexity), but it doesn't look like you need to call any fancy method now anyway.
oh, yea i like that idea.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:347
+                                     "type error");
+    return T(PyRefType::Borrowed, std::move(obj.get()));
+  } else {
----------------
labath wrote:
> Are you sure this std::move actually does anything -- I see no applicable rvalue constructor
It doesn't do anything yet, but it's conceptually correct and it will do something when refactor the PythonObject classes to get rid of the virtual methods.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68547