labath added subscribers: amccarth, zturner.
labath added a comment.
Thanks. I was just about to hit approve, but then I noticed one other thing...
:/ It seems that somebody (I guess it was @zturner) spent a lot of time in
creating the whole PythonObject hierarchy, and it's (worthwhile) goal seems to
be to enable the rest of the to code to avoid dealing with the python APIs
directly. However, here you're ignoring all of that, and jumping to the C
python interface directly.
I'd like to know what's your take on that? Have you considered basing these
file classes on the PythonDataObject stuff?
The main reason I mention that is your comment about PyErr_Occurred, and the
resulting boiler plate it produced. This seems like something that would be
nicely solved by some c++-like wrapper, which is exactly what the PythonObject
stuff is AFAICT.
Most of your interactions seem to be about calling methods. Would it be
possible to add a PythonDataObject wrapper for this (and any other frequently
used python API)? I'm hoping that we could have something like:
if (Expected<PythonInteger> i = m_py_obj.CallMethod<PythonInteger>(pybuffer))
l_bytes_written = i->GetInteger();
else
return Status(i.takeError());
instead of stuff like
auto bytes_written = Take<PythonObject>(
PyObject_CallMethod(m_py_obj, "write", "(O)", pybuffer.get()));
if (PyErr_Occurred())
return Status(llvm::make_error<PythonException>("Write"));
long l_bytes_written = PyLong_AsLong(bytes_written.get());
if (PyErr_Occurred())
return Status(llvm::make_error<PythonException>("Write"));
It doesn't look like adding this should be too hard (including the
template-based computation of the python signature), but it could go a long way
towards improving the readability, and it might actually fix other issues too
-- it seems that the PythonDataObjects currently completely ignore the
`PyErr_Occurred` stuff, which seems to be wrong, and it could actually be the
reason why @amccarth was unable to run the test suite with a debug python on
windows. Improving that, even if it's just for the stuff that's needed for your
work would be really helpful, as it would provide a pattern on how to fix the
remaining issues.
I'm sorry that I'm bringing this up so late in the review, but I am also
learning this stuff as we go along -- unfortunately, I don't think we have
anyone really knowledgeable about the python stuff still active in the lldb
community..
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1165
+ bool IsValid() const override {
+ GIL takeGIL;
+ return IsPythonSideValid() && Base::IsValid();
----------------
Maybe this GIL-talking can be done inside `IsPythonSideValid` (instead of two
IsValid methods)?
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1197
+ uint32_t options)
+ : OwnedPythonFile(file, borrowed, fd, options, false){};
+};
----------------
extra semicolon here too
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1369
+ const char *utf8 = PyUnicode_AsUTF8AndSize(pystring.get(), &size);
+ if (!utf8 || PyErr_Occurred())
+ return Status(llvm::make_error<PythonException>("Read"));
----------------
This is an example where _I think_ you may be using `PyErr_Occurred`
incorrectly. If python really asserts that PyErr_Occurred is always called,
then this is not right, as you will not call it if the result is null. There
are a couple of other examples like that in the code too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68188/new/
https://reviews.llvm.org/D68188
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
