Skip to content
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

Bump required C++ standard to c++17 #27012

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 6, 2023

PR summary

According to SciPy's Toolchain Roadmap [1], C++17 core features became available in 2022. This was concluded based on minimum compiler versions in manylinux2014 images, AIX systems, FreeBSD systems, and Windows. Note, some standard library features are not universally supported.

This commit does not move much code to C++17, just a couple of minor improvements that I could easily change.

[1] https://docs.scipy.org/doc/scipy/dev/toolchain.html#c-language-standards

PR checklist

@QuLogic QuLogic added this to the v3.9.0 milestone Oct 6, 2023
@QuLogic QuLogic added CI: Run cibuildwheel Run wheel building tests on a PR CI: Run cygwin Run cygwin tests on a PR labels Oct 6, 2023
Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

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

I see config changes and docstring changes, but no code changes. Are there more commits coming or is this it?

@@ -10,41 +10,40 @@
* */

const char* image_resample__doc__ =
"Resample input_array, blending it in-place into output_array, using an\n"
"affine transformation.\n\n"
R"""(Resample input_array, blending it in-place into output_array, using an affine transform.
Copy link
Member

Choose a reason for hiding this comment

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

extraneous paren

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is C++11 raw string literal syntax; it's R"<something>(...)<something>" where <something> is an optional string to prevent ambiguity if your string might contain )". I chose additional double quotes to make it look similar to a Python multiline string, but if that is confusing a different string could be chosen.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Yes, it is! Actually, let's keep it this way. I just haven't encountered it enough to recognize it for what it was.

@oscargus
Copy link
Contributor

oscargus commented Oct 6, 2023

This should be updated as well:

https://matplotlib.org/stable/devel/dependencies.html#c-compiler

@QuLogic
Copy link
Member Author

QuLogic commented Oct 6, 2023

I see config changes and docstring changes, but no code changes. Are there more commits coming or is this it?

From my point of view, that is all. This is primarily intended to allow new features by keeping up with the rest of the ecosystem, but I am far from an expert in modern C++ and what we could actually use. I expect @anntzer or @ianthomas23 will have a better idea of that.

At the very least, I do know that mplcairo uses C++17 and so if we're finally at the point where we can allow it, then we can start thinking about integrating/depending on that in the future. Also, C++14 will be useful for some of the overload handling code in #27011 (which is why it doesn't currently build.)

According to SciPy's Toolchain Roadmap [1], C++17 core features became
available in 2022. This was concluded based on minimum compiler versions
in `manylinux2014` images, AIX systems, FreeBSD systems, and Windows.
Note, some standard library features are not universally supported.

This commit does not move much code to C++17, just a couple of minor
improvements that I could easily change.

[1] https://docs.scipy.org/doc/scipy/dev/toolchain.html#c-language-standards
@WeatherGod
Copy link
Member

There is a test failure. A blitting test using tk. No clue if it is related to a change in compilers.

@tacaswell
Copy link
Member

Restarted the failed job. tk + blitting + osx is a known flaky test.

@ianthomas23
Copy link
Member

I expect @anntzer or @ianthomas23 will have a better idea of that.

One tangible benefit is use of std::optional (or possibly std::variant) when passing np.ndarray | None via pybind11 to C++ so that we can explicitly check the None-ness rather than relying on automatic conversion to a np.ndarray and inferring None based on the ndims or shape.

@WeatherGod WeatherGod merged commit caa750e into matplotlib:main Oct 10, 2023
49 of 50 checks passed
@QuLogic QuLogic deleted the cpp17 branch October 10, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Run cibuildwheel Run wheel building tests on a PR CI: Run cygwin Run cygwin tests on a PR Maintenance status: needs comment/discussion needs consensus on next step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants