Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
WeatherGod merged 1 commit intomatplotlib:mainfromQuLogic:cpp17
Oct 10, 2023
Merged

Conversation

QuLogic
Copy link
Member

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 inmanylinux2014 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

@QuLogicQuLogic added this to thev3.9.0 milestoneOct 6, 2023
@QuLogicQuLogic added CI: Run cibuildwheelRun wheel building tests on a PR CI: Run cygwinRun cygwin tests on a PR labelsOct 6, 2023
Copy link
Member

@WeatherGodWeatherGod 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
MemberAuthor

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'sR"<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
Member

This should be updated as well:

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

story645 and QuLogic reacted with thumbs up emoji

@QuLogic
Copy link
MemberAuthor

QuLogic commentedOct 6, 2023
edited
Loading

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 toallow 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 becameavailable in 2022. This was concluded based on minimum compiler versionsin `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 minorimprovements 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 ofstd::optional (or possiblystd::variant) when passingnp.ndarray | None via pybind11 to C++ so that we can explicitly check theNone-ness rather than relying on automatic conversion to anp.ndarray and inferringNone based on thendims orshape.

anntzer reacted with thumbs up emoji

@WeatherGodWeatherGod merged commitcaa750e intomatplotlib:mainOct 10, 2023
@QuLogicQuLogic deleted the cpp17 branchOctober 10, 2023 20:48
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@WeatherGodWeatherGodWeatherGod approved these changes

Assignees
No one assigned
Labels
CI: Run cibuildwheelRun wheel building tests on a PRCI: Run cygwinRun cygwin tests on a PRMaintenancestatus: needs comment/discussionneeds consensus on next step
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.

5 participants
@QuLogic@oscargus@WeatherGod@tacaswell@ianthomas23

[8]ページ先頭

©2009-2025 Movatter.jp