Skip to content
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.

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

py/qstr: Rename MP_QSTR_NULL to MP_QSTRnull to avoid intern collisions. #5142

Closed
wants to merge 1 commit into from

Conversation

nevercast
Copy link
Contributor

Fixes #5140

Use MP_QSTRnull instead of MP_QSTR_NULL in much the same way as MP_QSTRnumber_of was used, to avoid collisions.

This allows interning the string "NULL" without it clashing with the special QSTR. (Such as when freezing python code that contains the string "NULL").

@dpgeorge
Copy link
Member

Nice fix, exactly what I was thinking to do!

@nevercast nevercast marked this pull request as ready for review September 25, 2019 06:15
@nevercast
Copy link
Contributor Author

nevercast commented Sep 25, 2019

I'm not sure if it is necessary but I was thinking a frozen "NULL" string inside unix/coverage would have been nice to ensure this doesn't regress in the future. Though I doubt it would. Any thoughts on that?

( I have tested locally that using "NULL" in frozen modules for unix, stm32 and esp32 all pass. Its just not in this PR )

@dpgeorge
Copy link
Member

I'm not sure if it is necessary but I was thinking a frozen "NULL" string inside unix/coverage would have been nice to ensure this doesn't regress in the future.

Sounds good, would act as a reminder about this subtle point.

@nevercast
Copy link
Contributor Author

I can squash this if you like, left it as a seperate commit so that change was obvious.

@dpgeorge
Copy link
Member

I think it makes sense to rename the new frozen test file, so it could be used in the future for other qstr-related tests (eg call it frzqstr.py).

@dpgeorge
Copy link
Member

Thanks very much, merged in 7d58a19

@dpgeorge dpgeorge closed this Sep 26, 2019
@nevercast nevercast deleted the fix/qstr_null branch September 26, 2019 10:13
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Special treatment of null qstr means "NULL" is not valid in mpy-cross.
2 participants