Skip to content

PEP 007: Add C99 Flexible Array Member to the required feature list. #1349

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

Closed
wants to merge 1 commit into from

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Mar 31, 2020

PEP 007: Add C99 Flexible Array Member to the required feature list.

https://en.wikipedia.org/wiki/Flexible_array_member

Required to fix undefined C behavior.
https://bugs.python.org/issue40120

See also python/cpython#19232 which will use this.

@gpshead
Copy link
Member Author

gpshead commented Mar 31, 2020

So far testing on the PR including the buildbot tests have revealed no compilers that have a problem with this 21 year old syntax. If there were any stick in the mud i'd have expected it to be msvc, but even that seems happy.

Allowing this avoids undefined behavior. I'm assuming we shouldn't retroactively declare this for existing releases so I'm just stating 3.9 and beyond. Anyone who needs this fixed to build older versions can locally patch or find a compiler flag to avoid undefined behavior issues with the existing code.

@gpshead
Copy link
Member Author

gpshead commented Mar 31, 2020

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1. I assume the refleak failures are unrelated?

@vstinner
Copy link
Member

+1. I assume the refleak failures are unrelated?

There is a known leak: https://bugs.python.org/issue40115

@vstinner
Copy link
Member

I'm fine with the change, but I would prefer to first merge the PR and wait a few days to ensure that it doesn't break anything.

@gpshead
Copy link
Member Author

gpshead commented Apr 3, 2020

Given this feature is not compatible with C++ if we adopt it, it'd need to be on internal-only structs and never anything in a public .h file. :/

@pitrou
Copy link
Member

pitrou commented Apr 3, 2020

C++11 has thread_local, so we could use that if C++ is detected. However, one should ensure that the C and C++ variants are ABI-compatible.

@vstinner
Copy link
Member

vstinner commented Apr 3, 2020

Given this feature is not compatible with C++ if we adopt it, it'd need to be on internal-only structs and never anything in a public .h file. :/

That's a good motivation to make more and more structures opaque :-)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Sadly, if we put that in a public header file, it would break compilation on C++ (ex: pybind11) :-(

Maybe we should just avoid to use this feature in public header files, only in the C files?

@AA-Turner
Copy link
Member

Is this PR still live @gpshead / @vstinner, or can it be closed? The various linked PR / BPO at the start of the thread don't seem to have been updated in several months.

A

@vstinner
Copy link
Member

I suggest to close this PR for now. It can be reopened once https://bugs.python.org/issue40120 will be resolved.

https://bugs.python.org/issue40120 is a complex issue. There is a big concern about C++ compatibility :-(

@AA-Turner
Copy link
Member

Thanks Victor

A

@CAM-Gerlach
Copy link
Member

Seems like closing this has resulted in bpo-40120 going active again 😄

@JelleZijlstra
Copy link
Member

Closing things does wonders for getting people to care :)

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.

7 participants