Serhiy Storchaka added the comment: This issue is not just about readability or performance. It is about correctness, since none of callers check for failure of _PyUnicode_CompareWithId.
I just came to the same problem from other side (replacing PyUnicode_CompareWithASCII with PyUnicode_EqualToASCII or _PyUnicode_CompareWithId). Josh's idea in general LGTM, but there are few details: 1. None of callers check for failure of new functions as well. In boolean context the failure is interpreted as true. What is even worse, an exception is set, and this can cause a crash in debug build. If we don't want to rewrite all the uses of _PyUnicode_CompareWithId by adding the code for checking return value for error, we should make new function never failing. _PyUnicode_CompareWithId can fail if the first argument is not valid string or if there is no memory for allocating a Unicode object for identifier. I think it is better to return false value in these circumstances. If the first argument is not string, it isn't equal to an identifier. If it is an invalid string, it can't be equal too. If there is no memory for allocating a Unicode object for identifier, we should fallback to comparing the raw content (like in PyUnicode_CompareWithASCII). 2. It may be worth to replace _PyUnicode_FromId with the macro: #define _PyUnicode_FROM_ID(id) ((id)->object ? (id)->object : _PyUnicode_FromId(id)) Or rename _PyUnicode_FromId to _PyUnicode_FromIdInternal and _PyUnicode_FROM_ID to _PyUnicode_FromId? This would save us from rewriting a lot of code that uses _PyUnicode_FromId. 3. The name _PyUnicode_CompareWithIdEqual looks too long to me. What about _PyUnicode_EqualToId? 4. Pointers could be checked for equality before checking PyUnicode_CHECK_INTERNED(left). The former check is much cheaper than the latter. ---------- components: +Interpreter Core, Unicode nosy: +ezio.melotti, serhiy.storchaka type: -> behavior versions: +Python 3.6, Python 3.7 _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue21449> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
