FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-32571: Avoid raising unneeded AttributeError and silencing it C code. by serhiy-storchaka · Pull Request #5205 · python/cpython · GitHub

/ cpython Public

bpo-32571: Avoid raising unneeded AttributeError and silencing it C code. - #5205

Closed
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:getattr-without-error
Closed

bpo-32571: Avoid raising unneeded AttributeError and silencing it C code.#5205
serhiy-storchaka wants to merge 1 commit into
python:masterfrom
serhiy-storchaka:getattr-without-error

Conversation

serhiy-storchaka commented Jan 16, 2018
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.

https://bugs.python.org/issue32571

…ode.

Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.
Comment thread Include/object.h
/* 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 *);

1st1 Jan 16, 2018
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 **);
  • -1 error
  • 0 not found
  • 1 found

I'd also do this for _PyObject_GetAttrWithoutError which we committed today. /cc @methane

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Make sense. Go ahead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, I agree, it's even better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. "find" is far better than "without error".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like LookupAttrId. It sounds better.

1st1 Jan 17, 2018
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

BTW, I'd also remove the leading underscore from _PyObject_GetAttrWithoutError, making it PyObject_LookupAttr (and we keep the underscore for _PyObject_LookupAttrId).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

methane closed this Jan 25, 2018
serhiy-storchaka deleted the getattr-without-error branch September 22, 2018 19:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge performance Performance or resource usage skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL