[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: Sorry, I didn't have time to review this issue :-( Thanks Xiang and Serhiy for fixing the issue! I prefer the new API (don't ignore the error). -- ___ Python tracker _

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Changes by Serhiy Storchaka : -- resolution: -> fixed stage: commit review -> resolved status: open -> closed ___ Python tracker ___

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Roundup Robot added the comment: New changeset d06a6b0fd992 by Serhiy Storchaka in branch '3.6': Issue #28123: _PyDict_GetItem_KnownHash() now can raise an exception as https://hg.python.org/cpython/rev/d06a6b0fd992 New changeset 805467de22fc by Serhiy Storchaka in branch 'default': Issue #28123

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

INADA Naoki added the comment: LGTM. -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Serhiy Storchaka added the comment: LGTM. I'll commit the patch soon if there are no comments from other core developers. -- stage: patch review -> commit review ___ Python tracker

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: > If _PyDict_GetItem_KnownHash() returns an error, it is very likely that > following insertdict() with same key will return an error. Make sense. -- assignee: haypo -> serhiy.storchaka Added file: http://bugs.python.org/file45336/issue28123_v6.patch ___

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Serhiy Storchaka added the comment: If _PyDict_GetItem_KnownHash() returns an error, it is very likely that following insertdict() with same key will return an error. I would prefer to return an error right after _PyDict_GetItem_KnownHash() is failed. This would look more straightforward. ---

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Changes by Xiang Zhang : Added file: http://bugs.python.org/file45299/issue28123_v5.patch ___ Python tracker ___ ___ Python-bugs-list mailing

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: dict_merge was altered after the patch. I make it ignore explicitly the error now, to not affect former behaviour. Serhiy, I apply your suggestion to use _PyLong_AsByteArray for Py_hash_t, but I am not familiar with the API. It needs a review. -- Added f

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Changes by Serhiy Storchaka : -- priority: normal -> high stage: -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Serhiy Storchaka added the comment: Actually ignoring exceptions in _PyDict_GetItem_KnownHash causes a subtle difference between Python and C implementations. Making _PyDict_GetItem_KnownHash not ignoring exceptions looks right thing. But dict_merge expects that _PyDict_GetItem_KnownHash don't

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Changes by Stéphane Wirtel : -- assignee: -> haypo ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mai

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: Victor, v3 now applies your suggestion. -- Added file: http://bugs.python.org/file44713/issue28123_v3.patch ___ Python tracker ___

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Changes by Serhiy Storchaka : -- nosy: +rhettinger, serhiy.storchaka ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscr

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: Hah, Okay. I'll make the corresponding change then. -- ___ Python tracker ___ ___ Python-bugs-list mail

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: Xiang Zhang: "I am still afraid changing it may break knowledge of devs that are already familiar with dict APIs" Come on, the function is only called 4 times in the whole code base, and the function is private. Private means that we are free to break its behavi

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: Yes, ignoring exceptions is due to historical reasons. Although it's used rarely but I am still afraid changing it may break knowledge of devs that are already familiar with dict APIs. And what's more is there already exists two versions of getitem, one version w

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: If __eq__() raise an exception, _PyDict_GetItem_KnownHash() currently returns NULL and pass through the exception. To me, it looks like the correct behaviour. With your patch, it looks like the _PyDict_GetItem_KnownHash() clears the __eq__() exception: return

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: Update the patch with unittest. -- Added file: http://bugs.python.org/file44655/issue28123_v2.patch ___ Python tracker ___

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: Xiang Zhang added the comment: > How about let PyDict_GetItem call it? PyDict_GetItem() is like the most important function in term of performance for Python. If you want to touch it, you must benchmark it. I would prefer to keep it as it is. > Just like the r

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: How about let PyDict_GetItem call it? Just like the relationship of delitem and delitem_knownhash. You can see they share most codes. If we do that, it seems we can easily write a test(or there has already been a test) for it. --

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: > _PyDict_GetItem_KnownHash is not invoked by any other dict methods. Oh, I missed that. In this case, I suggest you to expose the function at Python level using the _testcapi module. And then use _testcapi._PyDict_GetItem_KnownHash() in test_dict. It would b

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: _PyDict_GetItem_KnownHash is not invoked by any other dict methods. So to achieve it in pure Python level, we have to rely on others modules and objects such as OrderedDict, lru_cache. Is it a good idea to rely on those? -- __

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: I understand the the code doesn't handle correctly lookup failures. Such failure is easy to trigger in pure Python using a custom __eq__() method for example. Can you please write an unit test for it? -- ___ Python

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

Xiang Zhang added the comment: Hmm, I thought this is trivial so I didn't. Now upload the file patch ;). -- keywords: +patch Added file: http://bugs.python.org/file44625/issue28123.patch ___ Python tracker

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

STINNER Victor added the comment: Please create a patch file and attach it to the issue, so we can review it more easily. -- ___ Python tracker ___ _

[issue28123] _PyDict_GetItem_KnownHash ignores DKIX_ERROR return

New submission from Xiang Zhang: _PyDict_GetItem_KnownHash should handle dk_lookup return value the same as PyDict_GetItem. BTW, it seems PyDict_GetItem can call _PyDict_GetItem_KnownHash to remove duplicate code, if you like, maybe another issue? diff -r 6acd2b575a3c Objects/dictobject.c ---