GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-32571: Avoid raising unneeded AttributeError and silencing it C code. by serhiy-storchaka · Pull Request #5205 · python/cpython

Conversation

serhiy-storchaka commented Jan 16, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.

https://bugs.python.org/issue32571

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-32571: Avoid raising unneeded AttributeError and silencing it C c…

a0d3e42

…ode.

Added _PyObject_GetAttrIdWithoutError() and used in appropriate cases.

serhiy-storchaka added performance

Performance or resource usage

skip news labels

Jan 16, 2018

serhiy-storchaka requested review from methane and vstinner

January 16, 2018 18:21

serhiy-storchaka requested review from 1st1 and rhettinger as code owners

January 16, 2018 18:21

the-knights-who-say-ni added the CLA signed label

Jan 16, 2018

bedevere-bot added the awaiting merge label

Jan 16, 2018

1st1 reviewed

Jan 16, 2018

View reviewed changes

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Jan 17, 2018

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Jan 17, 2018

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Jan 17, 2018

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jan 17, 2018

Copy link

Copy Markdown

Member Author

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 it can raise an error.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Jan 17, 2018

Copy link

Copy Markdown

Member

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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Jan 17, 2018

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Jan 17, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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 it PyObject_LookupAttr (and we keep the underscore for _PyObject_LookupAttrId).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Jan 17, 2018

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane closed this

Jan 25, 2018

serhiy-storchaka deleted the getattr-without-error branch

September 22, 2018 19:05

Reviewers

1st1

1st1 left review comments

vstinner

Awaiting requested review from vstinner

methane

Awaiting requested review from methane

rhettinger

Awaiting requested review from rhettinger

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge performance

Performance or resource usage

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

serhiy-storchaka commented Jan 16, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Jan 16, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

1st1 Jan 16, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

1st1 Jan 17, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.