/std:c++20 instead of /std:c++17 used for _wmimodule.cpp, but seems unnecessary

[CAM-Gerlach]

@python/windows-team

I noticed the VS project file _wmi.cxproj for _wmimodule.cpp added for 3.12 to support using WMI on Windows to get platform data in issue #89545 / PR #96289 specifies C++20 mode via the compiler flag /std:c++20. The flag (and partially-complete C++20 support) was only added in VS 2019 16.11, and produces a compiler warning in VS 2017 15.9 stating the flag was ignored.

This is the only file that requires it and there were no relevant hints in the issue, PR, or commit history as to why it was required. Despite the flag being ignored, there were no compiler errors or other warnings, and both the full test suite (minus a couple clearly unrelated issues) and running test_wmi and test_platform with -u all passed, and the same was true when I recompiled it with /std:c++17 (added in VS 2017 15.8 and used a couple other files), which naturally avoids the warning.

Therefore, it would seems specifying C++20 is unnecessary, and it can be changed to /std:c++17 to avoid compiler warnings and use a consistent C++ standard version with the other files, unless there's something I'm missing here (entirely possible, of course). Any reason this was added?

Linked PRs

• gh-99191: Use correct check for MSVC C++ version support in _wmimodule.cpp #100381

[vstinner]

I guess that /std:c++17 is fine and maybe @zooba just picked the default when he created the new project.

[zooba]

_wmimodule.cpp includes this explanation near the top of the file:

#if _MSC_VER >= 1929
// We can use clinic directly when the C++ compiler supports C++20
#include "clinic/_wmimodule.cpp.h"
#else
// Cannot use clinic because of missing C++20 support, so create a simpler
// API instead. This won't impact releases, so fine to omit the docstring.
static PyObject *_wmi_exec_query_impl(PyObject *module, PyObject *query);
#define _WMI_EXEC_QUERY_METHODDEF {"exec_query", _wmi_exec_query_impl, METH_O, NULL},
#endif

Basically, Argument Clinic generates files that aren't valid C++ prior to C++20, because it uses designated initializers. Since I know I'm releasing with a compiler that supports them, I opt-in, but to allow other people to build and debug, it's easy enough to create a METH_O API directly around the _impl function.

I don't think we have any way to detect the compiler version in the build files, but if there is, we could make the option conditional on having an old compiler.

[vstinner]

Basically, Argument Clinic generates files that aren't valid C++ prior to C++20, because it uses designated initializers.

Oh wow, now I recall all issues that I had when I wrote Lib/test/test_cppext.py! See Lib/test/_testcppext.cpp: it doesn't use designated initializers for this reason.

test_cppext checks the compatibility of the public Python C API with C++03 and C++11.

[vstinner]

Basically, Argument Clinic generates files that aren't valid C++ prior to C++20

See also #26080 and #85283 about modifying Argument Clinic to support the limited C API.

[vstinner]

For the specific case of _wmimodule.cpp, it's a single function which uses Argument Clinic. This function has a single argument. Writing the code by hand to parse its only argument should be easy, if someone has issues with C++17 and older.

[CAM-Gerlach]

Ah, thanks for the clarification!

Writing the code by hand to parse its only argument should be easy, if someone has issues with C++17 and older.

Isn't this what the current code already does, on versions of MSVC older than 19.29 (that don't support C++20)?

Though, I am curious—why indirectly check via this, and not _MSVC_LANG >= 202002L to check whether C++20 or greater is actually being targeted (since MSVC 19.29+ could be set to a lower version, and the /std:c++20 flag was only introduced in VS 2019 16.11, but 1929 is also the version reported in VS 2019 16.10, which doesn't support that flag?

I don't think we have any way to detect the compiler version in the build files, but if there is, we could make the option conditional on having an old compiler.

Yeah, they're static, and while there's /std:c++latest, that's only available on 16.10, and isn't exactly equivalent, and at least after a bit of investigation, there doesn't appear to be a straightforward way to do this. Given it has it just produces a warning that has no other apparent negative effect, I figure its best to just keep things how they are.

The one point of note is that it means that compilers that don't support C++20 will fall back to C++14 instead, rather than C++17 (though at least as of now, this doesn't appear to make an easily noticeable difference); perhaps /std:c++17 could be added before /std:c++20 in the arg string, so it falls back to C++17 if available instead of the default C++14?

[zooba]

Though, I am curious—why indirectly check via this, and not _MSVC_LANG >= 202002L to check whether C++20 or greater is actually being targeted

I wasn't aware of that variable, that's why.

perhaps /std:c++17 could be added before /std:c++20 in the arg string, so it falls back to C++17 if available instead of the default C++14?

If that works, it's fine by me.

But I thought the complaint was about additional warnings being seen by users on old versions of MSVC? Contradictory options usually result in more warnings (though this one may have been seen as a reasonable exception... the MSVC team is slowly learning that projects are built on purpose by people who aren't all on a homogenous setup maintained by their IT admin ;) )

[CAM-Gerlach]

Contradictory options usually result in more warnings (though this one may have been seen as a reasonable exception...

Oh; I'd seen the docs say "If CL encounters conflicting options, it uses the rightmost option" and I didn't see any additional warnings when I tested it on VS 2017, but it turns out that was just because that /std option isn't supported; when I added an additional option (\std:c++14) that is supported on 2017 (as would be the case on 2019+), it did issue a warning about the "contradiction" (though it still works ). Assuming 16.11+ still warns (the earliest that warning would be newly issued), then yeah it seems not worth doing this, unless C++17 features are also used (which they appear not to be currently).

However, I could make a PR to change the check to _MSVC_LANG >= 202002L, since the current check will break (resulting in the unsupported C++20 branch being taken and presumably resulting in a compiler error) on VS 16.10 and also if the build is invoked such that \std:c++20 is overridden or not passed.

[zooba]

However, I could make a PR to change the check to _MSVC_LANG >= 202002L

This seems like goodness regardless of any other changes. Ideally I'd prefer to feature test, but we may as well test the correct version if we're having a version test.

[CAM-Gerlach]

Sorry for the delay; finally opened PR #100381 to implement that.