Skip to content

bpo-38376: Fix build error when enabling assertions but not Py_DEBUG. #16594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpo-38376: Fix build error when enabling assertions but not Py_DEBUG. #16594

wants to merge 1 commit into from

Conversation

Yhg1s
Copy link
Member

@Yhg1s Yhg1s commented Oct 5, 2019 •
edited by bedevere-bot
Loading

_PyUnicode_CheckConsistency is now defined even without Py_DEBUG set, so get rid of the #define hack that was there to deal with calls in assert statements not explicitly guarded by Py_DEBUG.

https://bugs.python.org/issue38376

@Yhg1s
Fix build error when enabling assertions but not Py_DEBUG:
84875bb 
_PyUnicode_CheckConsistency is now defined even without Py_DEBUG set, so get
rid of the #define hack that was there to deal with calls in assert
statements not explicitly guarded by Py_DEBUG.
@Yhg1s Yhg1s requested a review from vsajip October 5, 2019 15:15
vstinner
vstinner reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

My intent was to also provide _PyObject_CheckConsistency() in release mode to help debugging GC crash, especially in visit_decref(). I wrote PR #16612 to fix my implementation.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

In short, _PyUnicode_CheckConsistency() is now always available in Python 3.8. I wrote PR #16623 to fix its definition in header files.

This change doesn't look correct. I'm not sure that _PyUnicode_CheckConsistency() definition depends on NDEBUG macro.

@vstinner
Copy link
Member

vstinner commented Oct 7, 2019

I merged my PR #16623 instead.

@vstinner vstinner closed this Oct 7, 2019
@Yhg1s
Copy link
Member Author

Yhg1s commented Oct 7, 2019

FYI, this change was intended to keep visibility parity, because the existing declaration (not definition) of _PyUnicode_CheckConsistency in Include/unicodeobject.h was already conditional on NDEBUG or Py_DEBUG. The only other declaration was in Include/internal/pycore_object.h, which wouldn't be visible to user code in the same way as before. Moving the declaration to Include/cpython/unicodeobject.h as you did also solved that same problem, because that header is included by Include/unicodeobject.h when not using the limited API. We should be all good now.

@Yhg1s Yhg1s deleted the pyunicode_checkconsistency branch October 7, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@vsajip vsajip vsajip approved these changes

Assignees
No one assigned
Labels
awaiting merge skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Yhg1s @vstinner @vsajip @the-knights-who-say-ni @bedevere-bot