Skip to content

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

Closed
ericsnowcurrently wants to merge 7 commits into
python:masterfrom
ericsnowcurrently:const-kwargs
Closed

bpo-36876: Make some static string literal arrays constant.#15760
ericsnowcurrently wants to merge 7 commits into
python:masterfrom
ericsnowcurrently:const-kwargs

Conversation

@ericsnowcurrently

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

Copy link
Copy Markdown
Member

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

@pgansslepganssle left a comment

Copy link
Copy Markdown
Member

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.

Comment threadModules/_datetimemodule.c Outdated
Comment threadModules/_decimal/_decimal.c Outdated
Comment threadModules/_decimal/_decimal.c Outdated
Comment threadModules/_decimal/_decimal.c
dec_##MPDFUNC(PyObject *self, PyObject *args, PyObject *kwds) \
{ \
static char *kwlist[] = {"other", "context", NULL}; \
static const char *kwlist[] = {"other", "context", NULL}; \

@pgansslepganssleSep 9, 2019

Copy link
Copy Markdown
Member

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

bedevere-bot commented Sep 9, 2019

Copy link
Copy Markdown

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
Comment threadModules/_multiprocessing/semaphore.c Outdated
pganssle
pganssle requested changes Sep 9, 2019
View reviewed changes

@pgansslepganssle left a comment

Copy link
Copy Markdown
Member

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.

Comment threadModules/mmapmodule.c
int access = (access_mode)ACCESS_DEFAULT;
DWORD flProtect, dwDesiredAccess;
static char *keywords[] = { "fileno", "length",
static const char *keywords[] = { "fileno", "length",

@pgansslepganssleSep 9, 2019

Copy link
Copy Markdown
Member

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.

Comment threadModules/posixmodule.c
/* Beware that "in" clashes with Python's own "in" operator keyword */
static char *keywords[] = {"out", "in",
static const char *keywords[] = {"out", "in",
"offset", "count",

@pgansslepganssleSep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alignment here again.

Comment threadModules/posixmodule.c
Py_ssize_t count;
PyObject *offobj;
static char *keywords[] = {"out", "in",
static const char *keywords[] = {"out", "in",

@pgansslepganssleSep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alignment again.

Comment threadModules/selectmodule.c Outdated
Comment threadPython/_warnings.c
{
static char *kwd_list[] = {"message", "category", "filename", "lineno",
static const char *kwd_list[] = {"message", "category", "filename", "lineno",
"module", "registry", "module_globals",

@pgansslepganssleSep 9, 2019

Copy link
Copy Markdown
Member

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

serhiy-storchaka commented Sep 10, 2019

Copy link
Copy Markdown
Member

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

serhiy-storchaka commented Sep 10, 2019

Copy link
Copy Markdown
Member

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

ericsnowcurrently commented Sep 10, 2019

Copy link
Copy Markdown
MemberAuthor

Hmm, this won't work out.

@georgthegreat

georgthegreat commented Feb 24, 2021

Copy link
Copy Markdown
Contributor

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

georgthegreat commented Feb 25, 2021

Copy link
Copy Markdown
Contributor

@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