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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
