gh-132775: Add _PyObject_GetXIDataWithFallback() #133482
Conversation
ericsnowcurrently
force-pushed
the
add-pyobject-getxidata-with-fallback
branch
from
e72c2af to
cf17342
Compare
Python/crossinterp.c
Outdated
| #ifdef Py_DEBUG | ||
| Py_UNREACHABLE(); |
There was a problem hiding this comment.
The #ifdef looks like a typo.
There was a problem hiding this comment.
It's there to make sure we abort on debug builds, but we have a fallback for non-debug builds.
There was a problem hiding this comment.
I'd prefer an explicit Py_FatalError() then. I think I can see this usage only in crossinterp.c (here, L1673, L1700).
There was a problem hiding this comment.
My intention is to not crash under normal usage, which is what Py_FatalError() would do.
There was a problem hiding this comment.
ah, Py_FatalError() instead of Py_UNREACHABLE()? I'm okay with that. It really should be unreachable through.
There was a problem hiding this comment.
I strongly prefer that Py_UNREACHABLE() be used when we care about the C compiler's optimization on non-debug builds (with no fallback). I won't be surprised even if it stops the abortion on debug builds in the future. Also, the double Py_DEBUG checks seem unnatural to me.
There was a problem hiding this comment.
That's fair. I'll switch it to explicitly Py_FatalError().
There was a problem hiding this comment.
Thanks. This is off-topic, but would one of the coming PRs address #133107 (comment)?
There was a problem hiding this comment.
Yeah, I'll be addressing that.
ericsnowcurrently
force-pushed
the
add-pyobject-getxidata-with-fallback
branch
2 times, most recently
from
04308b9 to
b9f1eea
Compare
ericsnowcurrently
force-pushed
the
add-pyobject-getxidata-with-fallback
branch
from
b9f1eea to
55ccaa0
Compare
| EXCEPTION, | ||
| OBJECT, |
There was a problem hiding this comment.
nit: indent. Also, cls is undefined at L726, 732:
assert not hasattr(mod, func.__name__), (cls, getattr(mod, func.__name__))| # Currently the "extra" attrs are not preserved | ||
| # (via __reduce__). | ||
| self.assertIs(type(exc1), type(exc2)) | ||
| #self.assert_exc_equal(grouped1, grouped2) |
There was a problem hiding this comment.
Is this still intentionally commented out? The assertion passed for me.
| def assert_equal_or_equalish(self, obj, expected): | ||
| cls = type(expected) | ||
| if cls.__eq__ is not object.__eq__: | ||
| # assert cls not in (types.MethodType, types.BuiltinMethodType, types.MethodWrapperType), cls |
There was a problem hiding this comment.
Ditto. See also:
L757:
# self.assert_roundtrip_equal(defs.TOP_FUNCTIONS)
L844, 997, 1463:
# UnicodeError: (None, msg, None, None, None),
| Py_FatalError("unsupported xidata fallback option"); | ||
| #endif | ||
| _PyErr_SetString(tstate, PyExc_SystemError, | ||
| "unsuppocted xidata fallback option"); |
| } | ||
|
|
||
| int | ||
| _PyObject_GetXIDataWithFallback(PyThreadState *tstate, |
There was a problem hiding this comment.
Could we replace the long name with _PyObject_GetXIData? I feel the existing _PyObject_GetXIData could be longer (e.g. _PyObject_GetXIDataNoFallback), as it is unpopular at all in newer PRs.
UPDATE: No, _PyObject_GetXIData should remain as-is.
There was a problem hiding this comment.
I'm not sure whether the fallback parameter is needed. Does this function support the following?
- call
getdata.basic()under the_PyXIDATA_FULL_FALLBACKcase - call
getdata.fallback()under the_PyXIDATA_XIDATA_ONLYcase
UPDATE: Yes, they are necessary.
There was a problem hiding this comment.
I take back the questions in this topic. Sorry for the noise.
(I currently interpret getdata.fallback as getdata.(with)fallback that can pass an option to tuple's items, and WithFallback as _PyFunction_GetXIData and _PyPickle_GetXIData.)
| typedef struct { | ||
| xidatafunc basic; | ||
| xidatafbfunc fallback; | ||
| } _PyXIData_getdata_t; |
There was a problem hiding this comment.
Does the layout imply that it would accept two functions in the future? If exclusive, is there any reason why it cannot be a void pointer (or xidatafunc) with a flag? (The C compilers could verify xidatafbfunc at REGISTER_FALLBACK().)
E: No change request from me as long as there are only two options.
| typedef int (*xidatafunc)(PyThreadState *tstate, PyObject *, _PyXIData_t *); | ||
| typedef int xidata_fallback_t; | ||
| #define _PyXIDATA_XIDATA_ONLY (0) | ||
| #define _PyXIDATA_FULL_FALLBACK (1) |
(This is based on gh-133481, thus only the last commit.)