Skip to content

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

Merged
acozzette merged 2 commits into
protocolbuffers:masterfrom
georgthegreat:fix-const
Mar 1, 2021
Merged

Fix some constness / char literal issues being found by MSVC standard conforming mode#8344
acozzette merged 2 commits into
protocolbuffers:masterfrom
georgthegreat:fix-const

Conversation

@georgthegreat

Copy link
Copy Markdown
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
Comment thread python/google/protobuf/pyext/descriptor_pool.cc Outdated
@georgthegreat

georgthegreat commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Collaborator

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

@georgthegreat georgthegreat left a comment

Copy link
Copy Markdown
Contributor Author

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.

static const char* kwlist[] = {"descriptor_db", 0};
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
Copy Markdown
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

georgthegreat commented Mar 1, 2021

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Collaborator

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.

3 participants