Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-32571: Avoid raising unneeded AttributeError and silencing it in C code (alt). #5222

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jan 17, 2018

Add _PyObject_LookupAttr() and _PyObject_LookupAttrId() and use them in appropriate cases instead of PyObject_GetAttr(), _PyObject_GetAttrId() and _PyObject_GetAttrIdWithoutError().

https://bugs.python.org/issue32571

…ode.

Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.
Return -1 and set *result == NULL if an error other than AttributeError
is raised.
*/
PyAPI_FUNC(int) _PyObject_LookupAttr(PyObject *, PyObject *, PyObject **);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename the method to PyObject_LookupAttr (no leading underscore). I'd use it in my libraries and the leading underscore would make me think that I shouldn't.

I also like Inada-san's suggestion to simplify the return value even further:

  • -1, if an unrelated error occurred during attribute lookup
  • 0 if no errors have occurred; *result will be set to NULL if the attribute was not found, and to a non-NULL value if it was found.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this API is experimental and should be kept private for some time. We are still discussing it, and after making some design decision we may change it in future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @serhiy-storchaka.
While I feel this API is nice, I'm not sure we should set
*result=NULL when returning -1 or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we care C API methods with a leading underscore the same as without? I've never seen us breaking the _ prefixed API to be honest. So leading underscore or not it's set in stone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1st1 Actually speaking, I broke _PyObject_GenericGetAttrWithDict(). I added one argument to suppress AttributeError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. The PR is LGTM.

@serhiy-storchaka serhiy-storchaka added performance Performance or resource usage skip news labels Jan 17, 2018
@methane
Copy link
Member

methane commented Jan 24, 2018

@serhiy-storchaka Would you merge this?
I want to use this API in #5273.

@1st1
Copy link
Member

1st1 commented Jan 24, 2018

I guess we can merge this ourselves if Serhiy isn't around. I'd like us to have this in 3.7.

@methane methane merged commit f320be7 into python:master Jan 25, 2018
@serhiy-storchaka serhiy-storchaka deleted the getattr-without-error-alt branch September 22, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants