Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
bpo-32571: Avoid raising unneeded AttributeError and silencing it C code.#5205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we have anint return value so that we don't need to callPyErr_Occurred()? IMHO, it's an API anti-pattern to require double checking whatNULL 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Make sense. Go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For usability, third argument should be filled with NULL when returning 1.
By it, caller can useif (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, I agree, it's even better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 itcan raise an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree. "find" is far better than "without error".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like LookupAttrId. It sounds better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW, I'd also remove the leading underscore from_PyObject_GetAttrWithoutError, making itPyObject_LookupAttr (and we keep the underscore for_PyObject_LookupAttrId).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Added
_PyObject_GetAttrIdWithoutError()and used in appropriate cases.https://bugs.python.org/issue32571