GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36876: Make some static string literal arrays constant. by ericsnowcurrently · Pull Request #15760 · python/cpython

Conversation

@ericsnowcurrently

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This gives us guarantees about immutability.

https://bugs.python.org/issue36876

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently added 2 commits September 9, 2019 12:24
@ericsnowcurrently
Fix all PyArg_ParseTupleAndKeywords calls.
5b20d3d
@ericsnowcurrently
Use "static const" in Python-ast.c.
dd1c235
@ericsnowcurrently ericsnowcurrently added the skip news label Sep 9, 2019
@ericsnowcurrently
ericsnowcurrently requested review from 1st1, abalkin, berkerpeksag, pganssle, rhettinger and skrah as code owners September 9, 2019 11:30
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 9, 2019
@bedevere-bot bedevere-bot added the awaiting core review label Sep 9, 2019
1st1
1st1 approved these changes Sep 9, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 9, 2019
pganssle
pganssle requested changes Sep 9, 2019
View reviewed changes

@pganssle pganssle left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_datetimemodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_decimal/_decimal.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_decimal/_decimal.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
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 pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 9, 2019
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Sep 9, 2019
pganssle
pganssle reviewed Sep 9, 2019
View reviewed changes
Comment thread Modules/_multiprocessing/semaphore.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pganssle
pganssle requested changes Sep 9, 2019
View reviewed changes

@pganssle pganssle left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Alignment here again.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/posixmodule.c
Py_ssize_t count;
PyObject *offobj;
static char *keywords[] = {"out", "in",
static const char *keywords[] = {"out", "in",

@pganssle pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Alignment again.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/selectmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
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 pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Alignment here again.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently and others added 5 commits September 10, 2019 16:21
@ericsnowcurrently @pganssle
Update Modules/_datetimemodule.c
bc86eab 
Co-Authored-By: Paul Ganssle <[email protected]>
@ericsnowcurrently @pganssle
Update Modules/_decimal/_decimal.c
83950ce 
Co-Authored-By: Paul Ganssle <[email protected]>
@ericsnowcurrently @pganssle
Update Modules/_decimal/_decimal.c
78da6ca 
Co-Authored-By: Paul Ganssle <[email protected]>
@ericsnowcurrently @pganssle
Update Modules/_multiprocessing/semaphore.c
2ea64ea 
Co-Authored-By: Paul Ganssle <[email protected]>
@ericsnowcurrently @pganssle
Update Modules/selectmodule.c
4bce0ff 
Co-Authored-By: Paul Ganssle <[email protected]>
@ericsnowcurrently

ericsnowcurrently commented Sep 10, 2019

Copy link
Copy Markdown
Member Author

Hmm, this won't work out.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently closed this Sep 10, 2019
@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):

  • brotli
  • rapidjson
  • annoy

Is there any way to continue working on the issue?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@georgthegreat georgthegreat mentioned this pull request Feb 25, 2021
Fix some constness / char literal issues being found by MSVC standard conforming mode protocolbuffers/protobuf#8344
Merged
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
[subinterpreters] Global C variables are a problem #81057
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@ericsnowcurrently @bedevere-bot @serhiy-storchaka @georgthegreat @1st1 @pganssle @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.