FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-36876: Make some static string literal arrays constant. by ericsnowcurrently · Pull Request #15760 · python/cpython · GitHub

/ cpython Public

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

Closed
ericsnowcurrently wants to merge 7 commits into
python:masterfrom
ericsnowcurrently:const-kwargs
ConversationCommits (7)ChecksFiles changed
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 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

pganssle requested changes Sep 9, 2019
View reviewed changes

pganssle 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Modules/_datetimemodule.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread 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}; \

pganssle Sep 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

pganssle 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

pganssle Sep 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Next two lines are also misaligned.

Comment thread Modules/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",

pganssle Sep 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Alignment here again.

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

pganssle Sep 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Alignment again.

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

pganssle Sep 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Alignment here again.

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 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 and others added 5 commits September 10, 2019 16:21
Update Modules/_datetimemodule.c
bc86eab 
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
Update Modules/_decimal/_decimal.c
83950ce 
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
Update Modules/_decimal/_decimal.c
78da6ca 
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
Update Modules/_multiprocessing/semaphore.c
2ea64ea 
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
Update Modules/selectmodule.c
4bce0ff 
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>

ericsnowcurrently commented Sep 10, 2019

Copy link
Copy Markdown
Member Author

Hmm, this won't work out.

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 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).

georgthegreat mentioned this pull request Feb 25, 2021
Merged
ericsnowcurrently mentioned this pull request Apr 10, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pganssle pganssle requested changes
1st1 1st1 approved these changes
abalkin Awaiting requested review from abalkin
berkerpeksag Awaiting requested review from berkerpeksag
rhettinger Awaiting requested review from rhettinger
skrah Awaiting requested review from skrah

Assignees

No one assigned

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL