Enable C++20

[targos]

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

[gengjiawen]

+1 from me. I like newer cpp features .

[RafaelGSS]

+1 from me too.

[mhdawson]

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

[gengjiawen]

@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]

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

[targos]

I'm trying to build locally with https://github.com/targos/node/commit/0065fd678f49d72437a870e85e2b9c02341f2c79

[targos]

I see a lot of [-Wambiguous-reversed-operator] in ICU. For example:

[308/3813] CXX obj/deps/icu-small/source/i18n/icui18n.pluralranges.o
In file included from ../../deps/icu-small/source/i18n/pluralranges.cpp:18:
In file included from ../../deps/icu-small/source/i18n/numrange_impl.h:15:
In file included from ../../deps/icu-small/source/i18n/number_formatimpl.h:14:
../../deps/icu-small/source/i18n/number_utils.h:53:17: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'const icu_72::MeasureUnit' and 'icu_72::MeasureUnit') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
    return unit == MeasureUnit();
           ~~~~ ^  ~~~~~~~~~~~~~
../../deps/icu-small/source/i18n/unicode/measunit.h:435:18: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
    virtual bool operator==(const UObject& other) const;
                 ^
1 warning generated.

[tniessen]

I'd like to see this happen. Related: https://github.com/nodejs/node/pull/44411#issuecomment-1229500140

[jasnell]

Yes please

[targos]

I think V8 built fine. There's an error in env.cc:

FAILED: obj/src/libnode.env.o
c++ -MMD -MF obj/src/libnode.env.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -D_GLIBCXX_USE_CXX11_ABI=1 -DNODE_OPENSSL_CONF_NAME=nodejs_conf -DNODE_OPENSSL_HAS_QUIC -D_DARWIN_USE_64_BIT_INODE=1 -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="arm64"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DNODE_USE_NODE_CODE_CACHE=1 -DHAVE_INSPECTOR=1 -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 '-DNODE_PLATFORM="darwin"' -DHAVE_OPENSSL=1 -DOPENSSL_API_COMPAT=0x10100000L -DBASE64_STATIC_DEFINE -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DNGHTTP2_STATICLIB -DNDEBUG -DL_ENDIAN -DOPENSSL_BUILDING_OPENSSL -DECP_NISTZ256_ASM -DKECCAK1600_ASM -DOPENSSL_BN_ASM_MONT -DOPENSSL_CPUID_OBJ -DPOLY1305_ASM -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DVPAES_ASM -DOPENSSL_PIC -DNGTCP2_STATICLIB -DNGHTTP3_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/base64/base64/include -I../../deps/googletest/include -I../../deps/histogram/src -I../../deps/uvwasi/include -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -I../../deps/openssl/openssl/crypto/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2 -I../../deps/ngtcp2 -I../../deps/ngtcp2/ngtcp2/lib/includes -I../../deps/ngtcp2/ngtcp2/crypto/includes -I../../deps/ngtcp2/ngtcp2/crypto -I../../deps/ngtcp2/nghttp3/lib/includes -O3 -gdwarf-2 -mmacosx-version-min=10.15 -arch arm64 -Wall -Wendif-labels -W -Wno-unused-parameter -Werror=undefined-inline -Wall -Wendif-labels -W -Wno-unused-parameter -Werror -std=gnu++20 -stdlib=libc++ -fno-rtti -fno-exceptions -fno-strict-aliasing  -c ../../src/env.cc -o obj/src/libnode.env.o
../../src/env.cc:1630:22: error: no matching constructor for initialization of 'node::DeserializeRequest'
  DeserializeRequest request{cb, {isolate(), holder}, index, info};
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/env.h:504:3: note: candidate constructor not viable: requires single argument 'other', but 4 arguments were provided
  DeserializeRequest(DeserializeRequest&& other) = default;
  ^
../../src/env.h:497:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 4 were provided
struct DeserializeRequest {
       ^
1 error generated.
[3657/3813] CXX obj/gen/libnode.node_javascript.o

Any suggestions?

[targos]

I created a draft PR: https://github.com/nodejs/node/pull/45427 Feel free to push fixes to it.

[gengjiawen]

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

[dharesign]

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

[targos]

Let's try to revert the problematic commit: https://github.com/nodejs/node/pull/45700

Edit: it breaks the MSVC build.

[dharesign]

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]

Relevant: https://devblogs.microsoft.com/oldnewthing/20190927-00/?p=102932

[dharesign]

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]

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]

wanted to learn the current progress

see https://github.com/nodejs/node/pull/45427. Mainly blocked by MSVC.

[targos]

I'll check the status of MSVC after https://github.com/nodejs/node/pull/46125

[gengjiawen]

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

[targos]

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

This is unfortunately still blocked on https://github.com/nodejs/build/issues/3317