Skip to content

gh-118124: Use static_assert() in Py_BUILD_ASSERT() on C11 #118398

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

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 29, 2024

@vstinner
Copy link
Member Author

See also PR gh-118125.

#define Py_BUILD_ASSERT_EXPR(cond) \
#if __STDC_VERSION__ >= 201112L
# define Py_BUILD_ASSERT_EXPR(cond) \
(sizeof(struct { _Static_assert(cond, #cond); int dummy; }), \
Copy link

Choose a reason for hiding this comment

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

Defining structs in expression is not valid C++. And _Static_assert is not a keyword in C++.

Copy link
Member Author

@vstinner vstinner Apr 29, 2024

Choose a reason for hiding this comment

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

Is __STDC_VERSION__ defined in C++?

Copy link
Member Author

Choose a reason for hiding this comment

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

For C++, we already have this code:

// gh-91782: On FreeBSD 12, if the _POSIX_C_SOURCE and _XOPEN_SOURCE macros are
// defined, <sys/cdefs.h> disables C11 support and <assert.h> does not define
// the static_assert() macro.
// https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255290
//
// macOS <= 10.10 doesn't define static_assert in assert.h at all despite
// having C11 compiler support.
//
// static_assert is defined in glibc from version 2.16. Compiler support for
// the C11 _Static_assert keyword is in gcc >= 4.6.
//
// MSVC makes static_assert a keyword in C11-17, contrary to the standards.
//
// In C++11 and C2x, static_assert is a keyword, redefining is undefined
// behaviour. So only define if building as C (if __STDC_VERSION__ is defined),
// not C++, and only for C11-17.
#if !defined(static_assert) && (defined(__GNUC__) || defined(__clang__)) \
     && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L \
     && __STDC_VERSION__ <= 201710L
#  define static_assert _Static_assert
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR to test explicitly for defined(__STDC_VERSION__).

Copy link

Choose a reason for hiding this comment

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

Is __STDC_VERSION__ defined in C++?

Ah, no. Missed that.

@vstinner
Copy link
Member Author

@namniav
Copy link

namniav commented Apr 29, 2024

Do you prefer leaving the macros unchanged for all C++ and C99, for reducing complexity?

Since you're working on this, can/should I close my PR?

@vstinner
Copy link
Member Author

Do you prefer leaving the macros unchanged for all C++ and C99, for reducing complexity?

_Static_assert() was added to C11. I don't think that we can expect it to be available on C99.

Is it available on C++?

@namniav
Copy link

namniav commented Apr 29, 2024

_Static_assert() was added to C11. I don't think that we can expect it to be available on C99.

Is it available on C++?

No. static_assert was added to C++11.

VLA was added to C99 (optional since C11) and is often available as compiler extension for older C versions and most C++ versions. There're tricks to also fix the two macros for them but would introduce some complexity in code, at least not as clean as your PR.

@vstinner
Copy link
Member Author

No. static_assert was added to C++11.

I updated the PR for C++. It now uses static_assert() on C++11 and newer.

@vstinner
Copy link
Member Author

The Docs CI job failed with:

Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Doc/tools/check-warnings.py", line 311, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/runner/work/cpython/cpython/Doc/tools/check-warnings.py", line 279, in main
    with Path("Doc/sphinx-warnings.txt").open(encoding="UTF-8") as f:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/pathlib.py", line 1013, in open
    return io.open(self, mode, buffering, encoding, errors, newline)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'Doc/sphinx-warnings.txt'

Use static_assert() in Py_BUILD_ASSERT() and Py_BUILD_ASSERT_EXPR()
on C11 and newer and C++11 and newer.

Add tests to test_cext and test_cppext.
@vstinner
Copy link
Member Author

The Docs CI job failed

I rebased my PR on the main branch to retrieve a fix. I also squashed my commits.

@vstinner
Copy link
Member Author

@namniav: So what do you think of this approach?

@namniav
Copy link

namniav commented Apr 30, 2024

It looks good to me. I'm not sure whether we should fix Py_BUILD_ASSERT_EXPR for older C versions and C++, I'm ok with either though.

@namniav
Copy link

namniav commented Apr 30, 2024

I meant to leave a comment yesterday but somehow started a review. There's no option to remove myself from reviewers, so I clicked "approved" to "finish the review".

@vstinner
Copy link
Member Author

It looks good to me. I'm not sure whether we should fix Py_BUILD_ASSERT_EXPR for older C versions and C++, I'm ok with either though.

An alternative is to remove it, but it seems like it's used outside by 3rd party projects.

@vstinner vstinner merged commit 587388f into python:main Apr 30, 2024
36 checks passed
@vstinner vstinner deleted the build_assert branch April 30, 2024 20:29
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
…hon#118398)

Use static_assert() in Py_BUILD_ASSERT() and Py_BUILD_ASSERT_EXPR()
on C11 and newer and C++11 and newer.

Add tests to test_cext and test_cppext.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants