-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-5891 extend C standard coverage up to C23 #1890
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
Conversation
|
Verified by this patch. The C Driver EVG Project settings have been updated to include the |
vector-of-bool
left a comment
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.
LGTM
| RHEL_DISTROS = [] | ||
| RHEL_DISTROS += ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6') | ||
| RHEL_DISTROS += ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0') | ||
| RHEL_DISTROS += ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4') | ||
| RHEL_DISTROS += ls_distro(name='rhel90', os='rhel', os_type='linux', os_ver='9.0') | ||
| RHEL_DISTROS += ls_distro(name='rhel91', os='rhel', os_type='linux', os_ver='9.1') | ||
| RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.2') | ||
| RHEL_DISTROS += ls_distro(name='rhel93', os='rhel', os_type='linux', os_ver='9.3') | ||
| RHEL_DISTROS += ls_distro(name='rhel94', os='rhel', os_type='linux', os_ver='9.4') | ||
| RHEL_DISTROS += ls_distro(name='rhel95', os='rhel', os_type='linux', os_ver='9.5') | ||
| RHEL_DISTROS += ls_distro(name='rhel8.9', os='rhel', os_type='linux', os_ver='8.7') | ||
| RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.0') |
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.
You can use the variadic splatting operator * in list/tuple literals (or ** in dict literals) to expand any iterable object into zero or more inline elements:
| RHEL_DISTROS = [] | |
| RHEL_DISTROS += ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6') | |
| RHEL_DISTROS += ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0') | |
| RHEL_DISTROS += ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4') | |
| RHEL_DISTROS += ls_distro(name='rhel90', os='rhel', os_type='linux', os_ver='9.0') | |
| RHEL_DISTROS += ls_distro(name='rhel91', os='rhel', os_type='linux', os_ver='9.1') | |
| RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.2') | |
| RHEL_DISTROS += ls_distro(name='rhel93', os='rhel', os_type='linux', os_ver='9.3') | |
| RHEL_DISTROS += ls_distro(name='rhel94', os='rhel', os_type='linux', os_ver='9.4') | |
| RHEL_DISTROS += ls_distro(name='rhel95', os='rhel', os_type='linux', os_ver='9.5') | |
| RHEL_DISTROS += ls_distro(name='rhel8.9', os='rhel', os_type='linux', os_ver='8.7') | |
| RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.0') | |
| RHEL_DISTROS = [ | |
| *ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6'), | |
| *ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0'), | |
| *ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4'), | |
| ... | |
| ] |
| if [[ "${CMAKE_GENERATOR:-}" =~ "Visual Studio" ]]; then | ||
| # MSBuild needs additional assistance. | ||
| # https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/ | ||
| export UseMultiToolTask=1 | ||
| export EnforceProcessCountAcrossBuilds=1 | ||
| fi |
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.
We should move to Ninja on Windows, eventually. Maybe someday.
kevinAlbs
left a comment
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.
LGTM. I really like the comprehensive std-c* tasks with the comments documenting the compiler versions.
| ('rhel94', 'gcc', None, [99, 11, 17, 23]), # GCC 11.4 (max: C2x) | ||
| ('rhel95', 'gcc', None, [99, 11, 17, 23]), # GCC 11.5 (max: C2x) | ||
|
|
||
| ('windows-vsCurrent', 'vs2017x64', None, [99, 11, 17, 'latest']), # Max: C17, clatest (C2x) |
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.
Change "latest" to "clatest"? The message in compile-std.sh refers to "clatest" (but checks for "latest"):
if [[ "${C_STD_VERSION}" == "latest" ]]; then
[[ "${CMAKE_GENERATOR:-}" =~ "Visual Studio" ]] || {
echo "C_STD_VERSION=clatest is only supported with Visual Studio generators" 1>&2There 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.
Mixed up the /std:clatest flag with the C_STD_VERSION=latest variable. Fixed.
* Ensure CC and CXX consistency, use CMAKE_GENERATOR for Visual Studio * Use ls_distro to reduce boilerplate * Sort std-matrix tasks by compiler and version * Support /std:clatest with MSVC * Exclude -Wc++98-compat warnings from public-header-warnings * Exclude -Wpre-c2x-compat warnings from public-header-warnings * Exclude -Wunsafe-buffer-usage from public-header-warnings * Address C2X compatibility issues due to _Bool macro expansions
* CDRIVER-5891 address implicit bool-to-null conversion in return statements (#1888) * CDRIVER-5891 extend C standard coverage up to C23 (#1890) * CDRIVER-5959 sync change streams unified spec tests with 0aee4aad (#1971) * CDRIVER-5304 set cluster time in test sessions (#2008) * Ignore -Wpre-c11-compat warnings when building public-header-warnings (#2027) * CDRIVER-6042 Migrate Python scripts from Poetry to Astral UV (#2039) * Migrate EVG task coverage to latest Debian, Ubuntu, and RHEL distros (#2044) * CDRIVER-6010 use `ec2.assume_role` for Azure KMS task (#2051) * CDRIVER-5971 Use Amazon ECR to obtain OCI images in EVG (#2058) * update Earthly version from 0.8.3 to 0.8.16 (#2072) --------- Co-authored-by: Kevin Albertson <[email protected]>
Summary
Resolves CDRIVER-5891.
Extends
std-matrixEvergreen tasks to support compilation with C99 through C23 using GCC, Clang, and MSVC.Drive-By EVG Config and Script Improvements
To facilitate the expansion of compilation coverage to newer versions of Visual Studio, compile scripts (beyond
std-matrixtasks) are updated to utilize the CMakeCMAKE_GENERATORandCMAKE_GENERATOR_PLATFORMenvironment variables to specify the Visual Studio version and target architecture (equivalent to-G+-A; similar changes to mongodb/mongo-cxx-driver#1082). The definition ofCCandCXXvariables are also moved into the config generator to ensure copmiler consistency (no need forif [[ "$CC" == "gcc" ]]-esque logic in scripts), including specific compiler version requests (e.g.clang-12). The list of distros indistros.pywas also refactored to use thels_distropattern to specify small+large distro pairs (similar changes to mongodb/mongo-cxx-driver#1082).Compiler Support
Reliably deducing compiler support for any given standard is a chore, especially given ongoing partial implementations of the C2x standard (as well as the C17 standard for older compiler versions). For example, support for
static_assertis already available as early as GCC 9, while other C2x features such as[[nodiscard]],__VA_OPT__, andnullptrare implemented by GCC 14, with more features such as#embedprovided by GCC 15, and etc. (see major version changelogs for 15.1, 14.1, 13.1, etc.). Similarly,static_assertand[[nodiscard]]are supported by Clang 9, with a scattering of C2x features being partially implemented across versions up to Clang 20 (technically even C11 standard support is still in a partial state).Instead of laborously deducing compiler support for language standards or features, detection of C standard support is completely deferred to CMake's built-in feature detection routines (regardless of what flag may be required:
-std=c11,-std=c1x,-std=c17,-std=c2x, or-std=c23). When a compiler does not support a given standard or feature, CMake will emit errors similar to the following (e.g. for GCC):The absence of C standards in the matrix for older compiler versions as proposed by this PR was entirely decided by the presence CMake errors similar to the one shown above.
Disabled Warnings
The
public-header-warnings(+ tests and examples) are added to the C standard task matrix. This motivated adding some new warning flags to be explicitly disabled for Clang with-Weverything:-Wc++98-compat-pedantic: we are not interested in continuing to support C++98 in either the C or C++ Driver. (Note: C++11 references the C99 spec.)-Wpre-c2x-compat: we actively test pre-C2x compatibility and do not need to rely on this warning.booldespite pre-C2x#define bool _Booland inclusion of C99<stdbool.h>is unnecessarily noisy.-Wunsafe-buffer-usage: too aggressive, warning on nearly any subscript of a pointer. Impractical to address this warning.C2x Boolean Compatibility
The following code:
and similarly:
produces the following compiler error with GCC 11 and GCC 12 given
-std=c2x:due to GCC 11 and GCC 12 defining boolean constants as:
instead of as a simple
1and0(as in C17 and prior) or as keywords (in GCC 13 and newer with-std=c2xor-std=c23). The"("mentioned in the error message is the first(in((_Bool)+1u).According to https://github.com/gcc-mirror/gcc, this change is due to this commit on Oct 29, 2020, which was motivated by this feature request. The
((_Bool)+1u)form was proposed by N2393 as an intermediate solution until the compiler has completed implementing the booleans as keywords. It seems<stdbool.h>for GCC 11 and GCC 12 implement this intermediate solution. These changes were later superceded by this commit on Sep 7, 2022, which remove the macros entirely for-std=c2x. This seems to correspond to the GCC 13 release and onwards which implemented the keyword-based solution.Because the language does not support contextually redefining
trueandfalseas "simple" macros wherever the BSON DSL macros may be expanded (that is, we cannot#definein a#define), a solution that attempts to preserve the use oftrueandfalseas unconditional DSL predicates was considered but rejected. Instead, the specialtrueandfalseDSL predicates are renamed to new and equivalent DSL predicates:alwaysandnever.Therefore, the code above must now be written instead as:
and similarly: