Skip to content

bpo-36876: Make some static string literal arrays constant.#15760

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
ericsnowcurrently wants to merge 7 commits into from
Closed

bpo-36876: Make some static string literal arrays constant. #15760

ericsnowcurrently wants to merge 7 commits into from

Conversation

@ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commented Sep 9, 2019•
edited by bedevere-bot
Loading

This gives us guarantees about immutability.

https://bugs.python.org/issue36876

1st1
1st1 approved these changes Sep 9, 2019
View reviewed changes
pganssle
pganssle requested changes Sep 9, 2019
View reviewed changes
Copy link
Member

@pgansslepganssle left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few formatting nits around whitespace I found.

Modules/_decimal/_decimal.c
dec_##MPDFUNC(PyObject *self, PyObject *args, PyObject *kwds) \
{\
static char *kwlist[] ={"other", "context", NULL}; \
static const char *kwlist[] ={"other", "context", NULL}; \
Copy link
Member

@pgansslepganssleSep 9, 2019

Choose a reason for hiding this comment

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

Few other alignment issues like the ones above - probably best to align the newline escapes as above.

@bedevere-bot
Copy link

bedevere-bot commented Sep 9, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pganssle
pganssle reviewed Sep 9, 2019
View reviewed changes
pganssle
pganssle requested changes Sep 9, 2019
View reviewed changes
Copy link
Member

@pgansslepganssle left a comment

Choose a reason for hiding this comment

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

Here are all the alignment issues I found. Hopefully the "suggestions" are the right number of spaces.

Modules/mmapmodule.c
intaccess= (access_mode)ACCESS_DEFAULT;
DWORDflProtect, dwDesiredAccess;
staticchar*keywords[] ={"fileno", "length",
staticconstchar*keywords[] ={"fileno", "length",
Copy link
Member

@pgansslepganssleSep 9, 2019

Choose a reason for hiding this comment

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

Next two lines are also misaligned.

Modules/posixmodule.c
/* Beware that "in" clashes with Python's own "in" operator keyword */
staticchar*keywords[] ={"out", "in",
staticconstchar*keywords[] ={"out", "in",
"offset", "count",
Copy link
Member

@pgansslepganssleSep 9, 2019

Choose a reason for hiding this comment

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

Alignment here again.

Modules/posixmodule.c
Py_ssize_tcount;
PyObject*offobj;
staticchar*keywords[] ={"out", "in",
staticconstchar*keywords[] ={"out", "in",
Copy link
Member

@pgansslepganssleSep 9, 2019

Choose a reason for hiding this comment

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

Alignment again.

Python/_warnings.c
{
staticchar*kwd_list[] ={"message", "category", "filename", "lineno",
staticconstchar*kwd_list[] ={"message", "category", "filename", "lineno",
"module", "registry", "module_globals",
Copy link
Member

@pgansslepganssleSep 9, 2019

Choose a reason for hiding this comment

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

Alignment here again.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 10, 2019

Changing char *kwlist[] to const char *kwlist[] is incompatible change, because PyArg_ParseTupleAndKeywords() takes char ** which is not compatible with const char **. The compiler should raise warnings or even errors about this. If not this this change would be made years ago.

It is possible to change the API, but it is long a complex process. It is a separate issue.

Adding consts in Python/Python-ast.c may be good, but this file is generated. You should modify the generating script.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 10, 2019

Note also that you can add two consts in static char *something[]: static const char * const something[]. One for C-strings, and other for an array.

See #15824 for changes in Python/Python-ast.c.

@ericsnowcurrently@pganssle
Update Modules/_datetimemodule.c
bc86eab
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
@ericsnowcurrently@pganssle
Update Modules/_decimal/_decimal.c
83950ce
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
@ericsnowcurrently@pganssle
Update Modules/_decimal/_decimal.c
78da6ca
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
@ericsnowcurrently@pganssle
Update Modules/_multiprocessing/semaphore.c
2ea64ea
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
@ericsnowcurrently@pganssle
Update Modules/selectmodule.c
4bce0ff
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
@ericsnowcurrently
Copy link
MemberAuthor

ericsnowcurrently commented Sep 10, 2019

Hmm, this won't work out.

@georgthegreat
Copy link
Contributor

georgthegreat commented Feb 24, 2021

At the time there is a bunch of forgotten const / const_cast / (char**) c-style casts everywhere in PyArg_ParseTupleAndKeywords invocations.

The list includes the following pip packages (but definitely not limited to):

Is there any way to continue working on the issue?

@georgthegreat
Copy link
Contributor

georgthegreat commented Feb 25, 2021

@serhiy-storchaka, could you, please, elaborate on

It is possible to change the API, but it is long a complex process. It is a separate issue.)

At the time the lack of const causes problems i. e. when compiling in MSVC standard conformance mode (which is default for /std:c++latest / /std:c++20).

@georgthegreatgeorgthegreat mentioned this pull request Feb 25, 2021
Merged
@ericsnowcurrentlyericsnowcurrently mentioned this pull request Apr 10, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgansslepgansslepganssle requested changes

@1st11st11st1 approved these changes

@abalkinabalkinAwaiting requested review from abalkin

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag

@rhettingerrhettingerAwaiting requested review from rhettinger

@skrahskrahAwaiting requested review from skrah

Assignees

No one assigned

Labels

awaiting changesskip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ericsnowcurrently@bedevere-bot@serhiy-storchaka@georgthegreat@1st1@pganssle@the-knights-who-say-ni