bpo-32571: Avoid raising unneeded AttributeError and silencing it C code. #5205
Conversation
…ode. Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.
| @@ -539,6 +539,7 @@ PyAPI_FUNC(int) _PyObject_IsAbstract(PyObject *); | |||
| /* Same as PyObject_GetAttr(), but don't raise AttributeError. */ | |||
| PyAPI_FUNC(PyObject *) _PyObject_GetAttrWithoutError(PyObject *, PyObject *); | |||
| PyAPI_FUNC(PyObject *) _PyObject_GetAttrId(PyObject *, struct _Py_Identifier *); | |||
| PyAPI_FUNC(PyObject *) _PyObject_GetAttrIdWithoutError(PyObject *, struct _Py_Identifier *); | |||
There was a problem hiding this comment.
Can we have an int return value so that we don't need to call PyErr_Occurred()? IMHO, it's an API anti-pattern to require double checking what NULL means.
PyAPI_FUNC(int) _PyObject_GetAttrIdWithoutError(
PyObject *, struct _Py_Identifier *, PyObject **);-1error0not found1found
I'd also do this for _PyObject_GetAttrWithoutError which we committed today. /cc @methane
There was a problem hiding this comment.
For usability, third argument should be filled with NULL when returning 1.
By it, caller can use if (val == NULL) instead of assigning return value to local variable.
if (_PyObject_GetAttrIdWithoutError((PyObject *)deque, &PyId___dict__, &func) < 0) {
// when error happens
return NULL;
}
if (func == NULL) {
// when `__dict__` not found.
Py_RETURN_NONE;
}
There was a problem hiding this comment.
Then I think It would be better to rename the function to _PyObject_LookupAttrId or _PyObject_FindAttrId. _PyObject_GetAttrIdWithoutError looks misleading bacause despite its name it can raise an error.
There was a problem hiding this comment.
I agree. "find" is far better than "without error".
There was a problem hiding this comment.
I like LookupAttrId. It sounds better.
There was a problem hiding this comment.
BTW, I'd also remove the leading underscore from _PyObject_GetAttrWithoutError, making it PyObject_LookupAttr (and we keep the underscore for _PyObject_LookupAttrId).
There was a problem hiding this comment.
Hm, "lookup" make sense too because there are _PyType_Lookup and _PyType_LookupId
and they doesn't set exception.
And +1 to make it public API.
Added
_PyObject_GetAttrIdWithoutError()and used in appropriate cases.https://bugs.python.org/issue32571