[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

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

lawrence_danna added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:261-264
 void PythonBytes::SetBytes(llvm::ArrayRef<uint8_t> bytes) {
   const char *data = reinterpret_cast<const char *>(bytes.data());
-  PyObject *py_bytes = PyBytes_FromStringAndSize(data, bytes.size());
-  PythonObject::Reset(PyRefType::Owned, py_bytes);
+  *this = Take<PythonBytes>(PyBytes_FromStringAndSize(data, bytes.size()));
 }
----------------
labath wrote:
> Do I understand correctly that the idea is to eventually replace these functions with constructor calls too?
yea


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:778
+  const char *script;
+  const char *function_name;
+  PythonCallable function;
----------------
labath wrote:
> I'm wondering if there's a way to avoid passing in the function name argument. Perhaps if the callable is returned as a result of the script ("def foo: ...; return foo")? WDYT?
I can't use return, but i can have the scripts put their function in a well-known output variable.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:789-790
+  llvm::Expected<PythonObject> operator()(Args &&... args) {
+    llvm::Error e = Init();
+    if (e)
+      return std::move(e);
----------------
labath wrote:
> if (llvm::Error e = ...)
Is that style considered normative?

I have a strong preference against putting function calls that are being run for their side effects inside an `if`.   


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:789-802
+  Expected<PythonObject> r = foo();
+
+  bool failed = !r;
+  ASSERT_TRUE(failed);
+
+  std::string backtrace;
+  llvm::handleAllErrors(r.takeError(), [&](const PythonException &E) {
----------------
labath wrote:
> Maybe:
> ```
> EXPECT_THAT_EXPECTED(foo(),
>   llvm::Failed<PythonException>(testing::Property(&PythonException::ReadBacktrace,
>      testing::ContainsRegex(...) )) ));
> ```
that doesn't seem to support multiple regexes, and is doing it 5 times really any more clear than just doing it manually?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69214