Moving tp_class access, and consistent fully-qualified naming for PyPy, to detail::get_tp_name #2520
Conversation
YannickJadoul
force-pushed
the
pypy-tp_name
branch
2 times, most recently
from
110b168 to
b4dc8e3
Compare
…y, to detail::get_tp_name
YannickJadoul
force-pushed
the
pypy-tp_name
branch
from
b4dc8e3 to
587ba52
Compare
| @@ -155,7 +155,7 @@ def test_internal_locals_differ(): | |||
| assert m.local_cpp_types_addr() != cm.local_cpp_types_addr() | |||
|
|
|||
|
|
|||
| @pytest.mark.xfail("env.PYPY") | |||
| @pytest.mark.xfail("env.PYPY and sys.pypy_version_info < (7, 3, 2)") | |||
There was a problem hiding this comment.
"Expect this to fail on PyPy < 7.3.2"
Didn't we decide to drop older versions already?
There was a problem hiding this comment.
7.3.2 is not out, yet. And it still fails on 7.3.1 :-)
There was a problem hiding this comment.
If 7.3.2 is out before this gets merged, yes, let's remove it
There was a problem hiding this comment.
(unrelated change, btw, but something I bumped into, when testing locally)
There was a problem hiding this comment.
I probably wouldn't drop it right away; approximately supporting 7.3.0+ would probably be ideal.
There was a problem hiding this comment.
Yes, that's probably best.
| @@ -24,6 +24,14 @@ PYBIND11_NAMESPACE_BEGIN(detail) | |||
| # define PYBIND11_SET_OLDPY_QUALNAME(obj, nameobj) setattr((PyObject *) obj, "__qualname__", nameobj) | |||
| #endif | |||
|
|
|||
| inline std::string get_tp_name(PyTypeObject *type) { | |||
| #if !defined(PYPY_VERSION) | |||
There was a problem hiding this comment.
What happens in 7.3.2? Does the line below start working?
There was a problem hiding this comment.
I don't think so, no. I believe the @pytest.mark.xfail("env.PYPY and sys.pypy_version_info < (7, 3, 2)") is unrelated (but I just came across it because I tested with 7.3.2.)
There was a problem hiding this comment.
Ugh .. I'd written this yesterday, but it was still a "pending review" ...
I think so, yes. Do the tests that I had to change (because PyPy is now consistent with CPython) count? |
There was a problem hiding this comment.
Thanks Yannick! I think this is good to merge, to help you finishing up #2349. I didn't have a chance to look into existing unit test coverage beyond the changes in test_class.py here, but we can look into that separately, later, or as the need arises.
YannickJadoul
force-pushed
the
pypy-tp_name
branch
from
3756f01 to
a22f2d7
Compare
|
OK, sounds good to me. Thanks, @rwgk! |
|
This seems safe to put in; would like to slowly keep a little momentum heading into 2.6.0. |
Increasing consistency, cfr. #2349 (comment)