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

Mon Oct 7 08:10:42 PDT 2019

labath added a comment.

I like the direction this is going in. Some questions about the implementation/interface inline..

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:44-45

+using llvm::Error;
+using llvm::Expected;
+
----------------
Please remove this. It kind of looks like this using declarations are local to this block, but they are not, and you are exposing these two new identifiers to anyone who happens to include this file. I am actually a proponent of importing these identifiers (and several others) into lldb_private, but that should be discussed separately. I started a discussion about that several months ago <http://lists.llvm.org/pipermail/lldb-dev/2019-April/014978.html>, but there wasn't universal consensus, so it kind of fizzled out.

Feel free to to restart that thread and/or to put these directives in a cpp file though.

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:104-110
+template <typename T> static T Take(PyObject *obj) {
+  return T(PyRefType::Owned, obj);
+}
+
+// Retain a reference you have borrowed, and turn it into
+// a PythonObject.
+template <typename T> static T Retain(PyObject *obj) {
----------------
What should be the behavior of these methods if `obj` is null? And what if obj is not of type `T` ? Right now, they will return an "empty" object, but this is the old behavior of the data objects classes (which was implemented back when we did not have Expected<T>), and I am not sure it is the right thing to do here. For the nullptr case, maybe we could handle that via an assertion (or even making these function take a `PyObject&` argument)? And a type mismatch seems like it could be best handled by returning an error instead of an empty object.

(Also, these functions shouldn't be static.)

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:104-122
+template <typename T> static T Take(PyObject *obj) {
+  return T(PyRefType::Owned, obj);
+}
+
+// Retain a reference you have borrowed, and turn it into
+// a PythonObject.
+template <typename T> static T Retain(PyObject *obj) {
----------------
labath wrote:
> What should be the behavior of these methods if `obj` is null? And what if obj is not of type `T` ? Right now, they will return an "empty" object, but this is the old behavior of the data objects classes (which was implemented back when we did not have Expected<T>), and I am not sure it is the right thing to do here. For the nullptr case, maybe we could handle that via an assertion (or even making these function take a `PyObject&` argument)? And a type mismatch seems like it could be best handled by returning an error instead of an empty object.
> 
> (Also, these functions shouldn't be static.)
Are there any cases where it is valid to "take" an object even if an exception has occured. Should we maybe move the "assert" into the previous two functions and delete this one? (From the name it does not seem too clear what is this function asserting, so it would be best to remove it altogether).

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:135
 public:
+  operator PyObject *() const { return m_py_obj; };
+
----------------
I think we should discourage people from passing these objects into the native APIs, so forcing them to use the existing `get()` methods seems sufficient to me.

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:259

-  bool IsNone() const;
+  operator bool() const { return IsValid() && !IsNone(); }

----------------
explicit

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:280
+  template <typename... Args>
+  Expected<PythonObject> CallMethod(const char *name, const char *format,
+                                    Args... args) {
----------------
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.

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:313-321
+  Expected<long long> AsLongLong() {
+    if (!m_py_obj)
+      return nullDeref();
+    assert(!PyErr_Occurred());
+    long long r = PyLong_AsLongLong(m_py_obj);
+    if (PyErr_Occurred())
+      return exception();
----------------
Should this maybe be a specialization of `AsType` for T = long long ? That might reduce your desire for monads...

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:323
+
+  Expected<bool> IsInstance(PyObject *cls) {
+    if (!m_py_obj || !cls)
----------------
I guess the RHS should by a PythonObject too..

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:347
+                                     "type error");
+    return T(PyRefType::Borrowed, std::move(obj.get()));
+  } else {
----------------
Are you sure this std::move actually does anything -- I see no applicable rvalue constructor

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:408
+  static Expected<PythonString> FromUTF8(llvm::StringRef string);
+  static Expected<PythonString> FromUTF8(const char *string);
+
----------------
this is not needed. const char * is implicitly convertible to a StringRef

================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:668
+
+template <typename T> T unwrapOrSetPythonException(llvm::Expected<T> expected) {
+  if (expected)
----------------
Add a comment about when should this be used. I guess it should be only done as the last thing before returning to python (?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68547