[Lldb-commits] [PATCH] D69133: eliminate nontrivial Reset(...) from TypedPythonObject

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think the conversion of different string types should be handled differently. Please see inline comment for details. Otherwise, this looks fine.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:326-338
   llvm::Expected<PythonObject> GetAttribute(const char *name) const {
     if (!m_py_obj)
       return nullDeref();
     PyObject *obj = PyObject_GetAttrString(m_py_obj, name);
     if (!obj)
       return exception();
     return python::Take<PythonObject>(obj);
----------------
This is the annoying part about interacting with external "c" apis in llvm. Llvm really likes to have StringRefs everywhere, but since they're not guaranteed to be null-terminated, it usually ends up requiring a string copy (`.str().c_str()`).
Having the wrapper APIs take `const char *` is not really a good option because that pushes the problem down to its users (and/or creates redundant strlen()s everywhere), whereas the purpose of these APIs should be to create an interface that interacts nicely with the rest of the codebase.

There are two ways  to get around that, depending on how much you want to optimize stuff:
# Just have your functions take a StringRef and call `.str().c_str()` when you need a null-terminated version of it
# Have your functions take a llvm::Twine. This class has a method <https://llvm.org/doxygen/classllvm_1_1Twine.html#a5d968bc337af0be1f18813553a046df6>, which enables you to avoid a string copy in the cases where the user passed in a null-terminated string, while avoiding to define multiple overloads of all your functions.
It is used as such:
```
auto my_foo_wrapper(const llvm::Twine &str) {
  llvm::SmallString<32 /*or whatever is the "typical" size*/> storage;
  llvm::StringRef s = str.toNullTerminatedStringRef(storage); // s is either the original const char *, or points to "storage"
  return foo(s.data() /*null terminated*/);
}
```
For this code I'd be fine with either option, but I'd like to avoid having two overloads of all functions (particularly if none of them is a StringRef).


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:401
 
-  void Reset(PyRefType type, PyObject *py_obj) {
-    Reset();
+  void Reset() { PythonObject::Reset(); }
+
----------------
Is this needed, given the "using" declaration above?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:406
+  TypedPythonObject(PyRefType type, PyObject *py_obj) {
+    m_py_obj = nullptr;
     if (!py_obj)
----------------
Isn't this already set by the superclass constructor?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:720-728
+// This is only here to help incrementally migrate old, exception-unsafe
+// code.
+template <typename T> T unwrapIgnoringErrors(llvm::Expected<T> expected) {
+  if (expected)
+    return std::move(expected.get());
+  llvm::consumeError(expected.takeError());
+  return T();
----------------
Could you put this into the "python" namespace, since we already have it? Otherwise, it looks like a generic lldb_private function (which I would very much like to avoid it becoming).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69133