Skip to content

bpo-46670: Define all macros for stringlib #31176

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

Merged
merged 1 commit into from
Feb 7, 2022
Merged

bpo-46670: Define all macros for stringlib #31176

merged 1 commit into from
Feb 7, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Feb 7, 2022

bytesobject.c, bytearrayobject.c and unicodeobject.c now define all
macros used by stringlib, to avoid using undefined macros.
Fix "gcc -Wundef" warnings.

https://bugs.python.org/issue46670

bytesobject.c, bytearrayobject.c and unicodeobject.c now define all
macros used by stringlib, to avoid using undefined macros.
Fix "gcc -Wundef" warnings.
@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

cc @serhiy-storchaka: I already merged my PR, but you may want to have a look ;-)

@serhiy-storchaka
Copy link
Member

Was it needed to define STRINGLIB_MUTABLE?

@vstinner
Copy link
Member Author

vstinner commented Feb 7, 2022

Was it needed to define STRINGLIB_MUTABLE?

I prefer to always define the macro to reduce the risk of bugs related to typos: #ifndef STRNIGLIB_MUTABLE is false because of a typo in the variable nane, whereas #if STRNIGLIB_MUTABLE == 0 emits a warning when Python is built with -Wundef: you can catch the typo.

stringlib uses many macros, the risk of forgetting to define a macro and the risk of a typo is high.

I adopted a smilar strategy in my #31171 change which replaces #ifndef PY_NO_SHORT_FLOAT_REPR with #if _PY_SHORT_FLOAT_REPR == 0. For this macro, it's worse because the macro is not define if you forget to include pycore_pymath.h!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants