classification
Title: Redundant code in _PyObject_GenericSetAttrWithDict
Type: enhancement Stage: commit review
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: benjamin.peterson, josh.r, martin.panter, pitrou, python-dev, serhiy.storchaka, xiang.zhang
Priority: low Keywords: patch

Created on 2016-04-13 02:37 by xiang.zhang, last changed 2016-04-17 17:33 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
_PyObject_GenericSetAttrWithDict.patch xiang.zhang, 2016-04-13 02:37 review
_PyObject_GenericSetAttrWithDict_2.patch serhiy.storchaka, 2016-04-15 09:44 review
Messages (9)
msg263297 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-04-13 02:37
It seems some code in _PyObject_GenericSetAttrWithDict is not necessary. There is no need to check data descriptor again using PyDescr_IsData. And the second if (f != NULL) {} will never function.
msg263341 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2016-04-13 16:57
LGTM. Had to check the definition of PyDescr_IsData to determine that checking the value from tp_descr_set for NULL was exactly what that macro does, but yeah, it looks like the first test was redundant, and f is never assigned outside that block, so the second block after the rest of the work is pointless; you'd never get there.
msg263449 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-15 05:30
This code mirrors code in the Get function, but that function inspects tp_descr_get (not set) instead. As I understand it, this is the difference between “data” descriptors and “non-data” descriptors. So I think the change is okay.
msg263463 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 09:44
LGTM. But there are other redundant code in _PyObject_GenericSetAttrWithDict(). Here is a patch that makes larger refactoring.
msg263467 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-04-15 10:22
It's a better work. And the code looks simpler now. I test it with the test suite and none fails (though some tests are skipped due to platform).
msg263478 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-15 11:38
Serhiy’s version looks good to me
msg263479 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 11:44
I'll defer committing the patch until some progress in issue26767 will be reached. May be _PyObject_GenericSetAttrWithDict() will have to rewrite again.
msg263619 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-17 17:32
New changeset e5149789e4ea by Serhiy Storchaka in branch 'default':
Issue #26745: Removed redundant code in _PyObject_GenericSetAttrWithDict.
https://hg.python.org/cpython/rev/e5149789e4ea
msg263620 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-17 17:33
Issue26767 looks too complex and resolving it needs much more rewriting.
History
Date User Action Args
2016-04-17 17:33:08serhiy.storchakasetstatus: open -> closed
resolution: remind -> fixed
messages: + msg263620
2016-04-17 17:32:15python-devsetnosy: + python-dev
messages: + msg263619
2016-04-15 11:44:59serhiy.storchakasetpriority: normal -> low
messages: + msg263479

assignee: serhiy.storchaka
resolution: remind
stage: patch review -> commit review
2016-04-15 11:38:08martin.pantersetmessages: + msg263478
2016-04-15 10:22:41xiang.zhangsetmessages: + msg263467
2016-04-15 09:44:37serhiy.storchakasetfiles: + _PyObject_GenericSetAttrWithDict_2.patch
type: enhancement
messages: + msg263463
2016-04-15 05:30:19martin.pantersetnosy: + martin.panter

messages: + msg263449
stage: patch review
2016-04-13 16:57:35josh.rsetnosy: + josh.r
messages: + msg263341
2016-04-13 05:21:50serhiy.storchakasetnosy: + serhiy.storchaka
2016-04-13 05:18:22xiang.zhangsetnosy: + pitrou, benjamin.peterson
2016-04-13 02:37:14xiang.zhangcreate