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

Enable C++20 #45402

Closed
targos opened this issue Nov 10, 2022 · 22 comments · Fixed by #45427
Closed

Enable C++20 #45402

targos opened this issue Nov 10, 2022 · 22 comments · Fixed by #45427
Labels
build Issues and PRs related to build files or the CI.

Comments

@targos
Copy link
Member

targos commented Nov 10, 2022

Chromium is making the move: https://bugs.chromium.org/p/chromium/issues/detail?id=1284275 / https://chromium-review.googlesource.com/c/chromium/src/+/3946562

Is it something we want to do too?

/cc @nodejs/cpp-reviewers

@targos targos added the build Issues and PRs related to build files or the CI. label Nov 10, 2022
@gengjiawen
Copy link
Member

+1 from me. I like newer cpp features .

@RafaelGSS
Copy link
Member

+1 from me too.

@mhdawson
Copy link
Member

@targos can we do that without bumping our compiler versions?

@gengjiawen
Copy link
Member

@targos can we do that without bumping our compiler versions?

we are currently using GCC 8.3, From https://en.cppreference.com/w/cpp/compiler_support#cpp20,
Gcc 10 likely the version we need to upgrade.

Also if v8 follows chromium decision, we have no choice but upgrade our toolchain.

@targos
Copy link
Member Author

targos commented Nov 11, 2022

Maybe our current compiler versions are enough. They all have some level of partial support of C++20.

targos added a commit to targos/node that referenced this issue Nov 11, 2022
@targos
Copy link
Member Author

targos commented Nov 11, 2022

I'm trying to build locally with targos@0065fd6

@targos

This comment was marked as duplicate.

@tniessen
Copy link
Member

I'd like to see this happen. Related: #44411 (comment)

@jasnell
Copy link
Member

jasnell commented Nov 11, 2022

Yes please

@targos

This comment was marked as duplicate.

@targos
Copy link
Member Author

targos commented Nov 11, 2022

I created a draft PR: #45427 Feel free to push fixes to it.

@gengjiawen
Copy link
Member

Not sure v8 can goes modules, if it goes. really look forward to build time performance.

@dharesign
Copy link
Contributor

As noted in #45694, we need a fix for CppHeapCreateParams as well. Please see the detail there.

@targos
Copy link
Member Author

targos commented Dec 1, 2022

Let's try to revert the problematic commit: #45700

Edit: it breaks the MSVC build.

@dharesign
Copy link
Contributor

OK, I figured out why it's breaking: it's because the CppHeapCreateParams struct is dllexport, which seems to cause it to fully instantiate the vector<> class, including parts that don't work.

Can reproduce: https://godbolt.org/z/8Wah9KbP3

Note that the error points to line 9 as the place the class is being instantiated, even though it's line 16 that's really causing it.

@dharesign
Copy link
Contributor

@dharesign
Copy link
Contributor

So unique_ptr<T> is not is_copy_constructible, but vector<unique_ptr<T>> is. It seems that this is a deliberate design choice that enables vector to be instantiated with incomplete types.

The only way to then export such a non-copyable class is to have the copy constructors explicitly deleted.

Doing that breaks the C++20 aggregate initialization per https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf

I guess the most reasonable way forward is probably to give CppHeapCreateParams real constructors and not rely on aggregate initialization.

Thoughts @mlippautz?

@anonrig
Copy link
Member

anonrig commented Jan 10, 2023

While I'm working on Ada, I am currently bounded by C++17 due to the possibility for being a suitable candidate for Node. Some nice features are coming with C++20, and I wanted to learn the current progress, timeline, and missing things required to make this change. I'd be happy to help.

@gengjiawen
Copy link
Member

wanted to learn the current progress

see #45427. Mainly blocked by MSVC.

@targos
Copy link
Member Author

targos commented Jan 11, 2023

I'll check the status of MSVC after #46125

@gengjiawen
Copy link
Member

Upstream issue in v8: https://crbug.com/1377771

targos added a commit to targos/node that referenced this issue Mar 30, 2023
targos added a commit to targos/node that referenced this issue Mar 31, 2023
targos added a commit to targos/node that referenced this issue Apr 20, 2023
targos added a commit to targos/node that referenced this issue May 5, 2023
targos added a commit to targos/node that referenced this issue May 22, 2023
targos added a commit to targos/node that referenced this issue Jun 12, 2023
targos added a commit to targos/node that referenced this issue Aug 2, 2023
targos added a commit to targos/node that referenced this issue Sep 5, 2023
targos added a commit to targos/node that referenced this issue Sep 20, 2023
targos added a commit to targos/node that referenced this issue Sep 27, 2023
@targos
Copy link
Member Author

targos commented Nov 14, 2023

Chromium now officially supports C++20: https://chromium-review.googlesource.com/c/chromium/src/+/4217486

This is unfortunately still blocked on nodejs/build#3317

targos added a commit to targos/node that referenced this issue Dec 10, 2023
targos added a commit to targos/node that referenced this issue Dec 11, 2023
targos added a commit to targos/node that referenced this issue Jan 1, 2024
targos added a commit to targos/node that referenced this issue Jan 8, 2024
targos added a commit to targos/node that referenced this issue Mar 15, 2024
targos added a commit that referenced this issue Apr 19, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: #45402
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Apr 19, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: nodejs#45402
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Apr 21, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: nodejs#45402
targos pushed a commit to targos/node that referenced this issue Apr 21, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: nodejs#45402
targos pushed a commit to targos/node that referenced this issue Apr 22, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: nodejs#45402
nodejs-github-bot pushed a commit that referenced this issue Apr 22, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: #45402
PR-URL: #52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS pushed a commit that referenced this issue Apr 22, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: #45402
PR-URL: #52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos added a commit to targos/node that referenced this issue May 3, 2024
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this issue May 8, 2024
Closes: nodejs#45402
PR-URL: nodejs#45427
Fixes: nodejs#45402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Steven R Loomis <srl295@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
SophonieBouye pushed a commit to SophonieBouye/node that referenced this issue Jun 20, 2024
Closes: nodejs#45402
PR-URL: nodejs#45427
Fixes: nodejs#45402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Steven R Loomis <srl295@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
Our Linux build infra is not ready for it yet,
but V8 is making it difficult to compile on Windows
without it.

Refs: nodejs#45402
PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
Closes: nodejs#45402
PR-URL: nodejs#45427
Fixes: nodejs#45402
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Steven R Loomis <srl295@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants