Skip to content
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

Fix some constness / char literal issues being found by MSVC standard conforming mode #8344

Merged
merged 2 commits into from Mar 1, 2021

Conversation

georgthegreat
Copy link
Contributor

This one is somewhat tricky.

According to C Language Standard "string" has type char[7] (that is, 6 letters + null terminator).

According to C++ Standard, however, it has its natural type of const char[7] (see cppreference).

An attempt to write char* ptr = "string";, however, does not cause any errors when compiled by clang or gcc.
MSVC has /permissive- flag which is off by default, unless /std:c++latest is used.

This PR fixes issues caused by this type mismatch.

@google-cla google-cla bot added the cla: yes label Feb 25, 2021
@georgthegreat
Copy link
Contributor Author

georgthegreat commented Feb 25, 2021

@acozzette, I am fine with adjusting the code according to the style guide, but is there any chance that Google, being a visionary sponsor of PSF, could make something to fix this issue right in the Python itself?

These const_cast's are nasty. Removing them in the future would require adding #ifdef checking for current Python version in every line this method is invoked.

@georgthegreat georgthegreat changed the title Fix some constness / char literal issues being found by MSVC standard conforming modea Fix some constness / char literal issues being found by MSVC standard conforming mode Feb 25, 2021
@acozzette
Copy link
Member

@georgthegreat This would definitely be nice to fix within Python itself, but unfortunately I don't think I can help much there because I'm not really connected to anyone would be in a good position to work on that.

Copy link
Contributor Author

@georgthegreat georgthegreat left a comment

Choose a reason for hiding this comment

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

Ok. Fixing this in python will require changes in many opensource projects anyway. It's better not to block on this.

I have changed to const_cast, as requested.

PyObject* py_database = NULL;
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|O", kwlist, &py_database)) {
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|O", (char**)kwlist, &py_database)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is another tricky thing.

CPython C API was using char* for const literals in py2, with many problems being fixed (that is, replaced with const char*) in Python3. This one, however, remains unfixed (this PR attempted to fix it, but did not get to the merge commit and looks abandoned).

Similar crutchy remove_const-casts can be found in various open source projects:

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

It might be better to fix python problem before merging this PR.

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Mar 1, 2021

@acozzette, none of the above links claiming that Kokoro build failed work for me.

Could you, please, help me to understand failure reasons?

@acozzette
Copy link
Member

Sorry about that, those failures were caused by an unrelated issue that should be fixed now. Let me try running the tests again.

@acozzette acozzette merged commit e9091e6 into protocolbuffers:master Mar 1, 2021
@georgthegreat georgthegreat deleted the fix-const branch March 2, 2021 08:51
georgthegreat added a commit to georgthegreat/protobuf that referenced this pull request May 5, 2021
acozzette pushed a commit that referenced this pull request May 6, 2021
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.

None yet

3 participants