build: switch to C++23

[targos]

Following Chromium.

Closes: #61125
Refs: https://issues.chromium.org/issues/388070065

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[targos]

Shows an error in quic code (@jasnell):

In file included from /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/memory:78:
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/unique_ptr.h:91:16: error: invalid application of 'sizeof' to an incomplete type 'node::quic::Stream::Outbound'
   91 |         static_assert(sizeof(_Tp)>0,
      |                       ^~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/unique_ptr.h:205:4: note: in instantiation of member function 'std::default_delete<node::quic::Stream::Outbound>::operator()' requested here
  205 |           _M_deleter()(__old_p);
      |           ^
/usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/unique_ptr.h:503:7: note: in instantiation of member function 'std::__uniq_ptr_impl<node::quic::Stream::Outbound, std::default_delete<node::quic::Stream::Outbound>>::reset' requested here
  503 |         _M_t.reset(std::move(__p));
      |              ^
../src/quic/streams.cc:324:23: note: in instantiation of member function 'std::unique_ptr<node::quic::Stream::Outbound>::reset' requested here
  324 |     stream->outbound_.reset();
      |                       ^
../src/quic/streams.h:294:9: note: forward declaration of 'node::quic::Stream::Outbound'
  294 |   class Outbound;
      |         ^
1 error generated.

[targos]

I opened ngtcp2/ngtcp2#1970 for the ngtcp2 error.

[ShenHongFei]

temporary workaround.

Aside from this, there are no other problems, and it compiles successfully.

diff --git a/src/quic/streams.h b/src/quic/streams.h
index c230815d78e..3c9fb100219 100644
--- a/src/quic/streams.h
+++ b/src/quic/streams.h
@@ -292,7 +292,12 @@ class Stream final : public AsyncWrap,
   struct PendingHeaders;
 
   class Outbound;
-
+  
+  
+  struct OutboundDeleter {
+    void operator() (Outbound* ptr) const;
+  };
+  
   // Gets a reader for the data received for this stream from the peer,
   BaseObjectPtr<Blob::Reader> get_reader();
 
@@ -335,7 +340,7 @@ class Stream final : public AsyncWrap,
   AliasedStruct<Stats> stats_;
   AliasedStruct<State> state_;
   BaseObjectWeakPtr<Session> session_;
-  std::unique_ptr<Outbound> outbound_;
+  std::unique_ptr<Outbound, OutboundDeleter> outbound_;
   std::shared_ptr<DataQueue> inbound_;
 
   // If the stream cannot be opened yet, it will be created in a pending state.
diff --git a/src/quic/streams.cc b/src/quic/streams.cc
index 8fe5b72ce1f..8fa9c264c4f 100644
--- a/src/quic/streams.cc
+++ b/src/quic/streams.cc
@@ -739,6 +739,11 @@ class Stream::Outbound final : public MemoryRetainer {
   size_t uncommitted_ = 0;
 };
 
+
+void Stream::OutboundDeleter::operator() (Stream::Outbound* ptr) const {
+    delete ptr;
+}
+
 // ============================================================================
 
 #define V(name, key, no_side_effect)                                           \
@@ -1038,7 +1043,7 @@ void Stream::set_outbound(std::shared_ptr<DataQueue> source) {
   if (!source || !is_writable()) return;
   Debug(this, "Setting the outbound data source");
   DCHECK_NULL(outbound_);
-  outbound_ = std::make_unique<Outbound>(this, std::move(source));
+  outbound_ = std::unique_ptr<Outbound, OutboundDeleter>(new Outbound(this, std::move(source)));
   state_->has_outbound = 1;
   if (!is_pending()) session_->ResumeStream(id());
 }

[dygabo]

is this planned to be backported to v25 and v24 as well or it stays for v26 only?

[targos]

@nodejs/quic PTAL at #61132 (comment).
I guess it should be possible to contribute a fix that works with both C++20 and C++23 in a separate PR.

[targos]

Now that quic is behind a compile-time flag, this should pass.

[targos]

@nodejs/build does it have to be semver-major?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/70997/

[richardlau]

@nodejs/build does it have to be semver-major?

I think we've always treated these as semver-major as common.gypi is also used by node-gyp to build addons.
For moving to C++20 we had a semver-major PR and a separate PR for v22.x.

[targos]

Looks like this triggers an error with GCC 14:

https://ci.nodejs.org/job/node-test-commit-linux/68207/nodes=alpine-latest-x64/console

10:55:27 In file included from ../deps/v8/src/compiler/js-inlining.cc:5:
10:55:27 In file included from ../deps/v8/src/compiler/js-inlining.h:8:
10:55:27 In file included from ../deps/v8/src/compiler/graph-reducer.h:8:
10:55:27 In file included from ../deps/v8/src/base/compiler-specific.h:8:
10:55:27 In file included from ../deps/v8/include/v8config.h:24:
10:55:27 In file included from /usr/lib/gcc/x86_64-alpine-linux-musl/14.2.0/../../../../include/c++/14.2.0/memory:69:
10:55:27 In file included from /usr/lib/gcc/x86_64-alpine-linux-musl/14.2.0/../../../../include/c++/14.2.0/bits/stl_uninitialized.h:63:
10:55:27 /usr/lib/gcc/x86_64-alpine-linux-musl/14.2.0/../../../../include/c++/14.2.0/bits/stl_algobase.h:1616:12: error: no matching function for call to object of type 'std::_Bind_front<bool (v8::internal::wasm::TypeCanonicalizer::CanonicalEquality::*)(const v8::internal::wasm::TypeCanonicalizer::CanonicalType &, const v8::internal::wasm::TypeCanonicalizer::CanonicalType &) const, const v8::internal::wasm::TypeCanonicalizer::CanonicalEquality *>'
10:55:27  1616 |         if (!bool(__binary_pred(*__first1, *__first2)))
10:55:27       |                   ^~~~~~~~~~~~~

[targos]

Config for Windows doesn't seem to work:

https://ci.nodejs.org/job/node-compile-windows/65002/nodes=win-vs2022_clang/console

09:35:03 clang-cl : warning : argument unused during compilation: '/std:c++23preview' [-Wunused-command-line-argument] [C:\workspace\node-compile-windows\node\tools\icu\icutools.vcxproj]

/cc @StefanStojanovic

[miladfarca]

fyi V8 still builds with c++20, for now: http://crrev.com/c/7237976

[StefanStojanovic]

Config for Windows doesn't seem to work:

https://ci.nodejs.org/job/node-compile-windows/65002/nodes=win-vs2022_clang/console

09:35:03 clang-cl : warning : argument unused during compilation: '/std:c++23preview' [-Wunused-command-line-argument] [C:\workspace\node-compile-windows\node\tools\icu\icutools.vcxproj]

/cc @StefanStojanovic

VS2022 comes with Clang 19.x, which doesn't support '/std:c++23preview'. VS2026 comes with Clang 20.x, which supports it, so the way I see it, the only way to make this happen is by using VS2022.

[targos]

I guess you meant "by using VS2026" :) What do you think about it?

[StefanStojanovic]

I guess you meant "by using VS2026" :) What do you think about it?

Correct, it was a typo. The compilation works on VS2026, as far as I know (tested on main), so there is no problem there. Infra (both test and release) is where most (if not all) of the work would have to be done - creating new machines, setting them up, connecting to Jenkins, etc. I've done this for VS2022, so it should be very similar for VS2026. The question I have is tied to the ETA - is there some date by which we have to transition? I saw from a comment that for V8, c++20 is still working, so is this mandatory change for us, or just something that would be good to have?

[targos]

I wasn't aware that V8 didn't make the switch. We can wait for them.

[StefanStojanovic]

I was thinking in this way - once v20 goes EOL in a few months, we can start removing VS2019 compilation machines, as they will no longer be needed (or should we keep them for some more time?). Once that's being worked on, we can start adding VS2026 machines in parallel (basically replacing 2019 with 2026), thus keeping the number of VMs the same. Does this sound reasonable to you?

[targos]

Sounds good to me!