Page MenuHomePhabricator

[Python] Update PyString_FromString() to work for python 2 and 3.
ClosedPublic

Authored by davide on Jan 9 2019, 1:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Jan 9 2019, 1:21 PM
JDevlieghere accepted this revision.Jan 9 2019, 1:25 PM

There's two more mentions of this function:

  • unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  • source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

I guess these have to be updated as well? Would it be worth to define a PYSTRING_FROM_STRING macro?

This revision is now accepted and ready to land.Jan 9 2019, 1:25 PM
zturner requested changes to this revision.Jan 9 2019, 1:34 PM

There's two more mentions of this function:

  • unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
  • source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

I guess these have to be updated as well? Would it be worth to define a PYSTRING_FROM_STRING macro?

I think it's the other way around. Definitely the one in PythonDataObjects.cpp should remain, and the code in this patch should be updated to use PythonString, which already abstracts all this away. In general we should not need explicit version checks anywhere outside of PythonDataObjects.cpp, as the whole purpose of that is to abstract away the differences so we don't need ifdefs in our code.

lldb/scripts/Python/python-swigsafecast.swig
28–39 ↗(On Diff #180912)

This entire function can just be deleted, it's not used.

lldb/scripts/Python/python-wrapper.swig
829–835 ↗(On Diff #180912)

Instead of the version checks, use this:

PythonString str(callee_name);
PyObject *result = PyObject_CallMethodObjectArgs(implementor, str.GetObject(), arg, nullptr);
This revision now requires changes to proceed.Jan 9 2019, 1:34 PM
zturner added inline comments.Jan 9 2019, 1:40 PM
lldb/scripts/Python/python-wrapper.swig
834 ↗(On Diff #180912)

Note that there appears to be an object leak here, as the str_arg is never released. Changing to PythonString should also fix that bug as a free side effect.

davide updated this revision to Diff 180924.Jan 9 2019, 2:03 PM

Address comments

zturner accepted this revision.Jan 9 2019, 2:11 PM
zturner added inline comments.
lldb/scripts/Python/python-wrapper.swig
827 ↗(On Diff #180924)

This looks like another possible object leak. The documentation for PyObject_CallMethodObjArgs doesn't mention whether it takes ownership of the argument, but I think it wouldn't make sense for it to do so, because you're just calling a function. In any case, just make sure the current change doesn't regress the test suite.

This revision is now accepted and ready to land.Jan 9 2019, 2:11 PM
This revision was automatically updated to reflect the committed changes.