node
node copied to clipboard
nodejs
src: modernize cleanup queue to use c++20
Getting the idea from @jasnell on https://github.com/nodejs/node/pull/56059, let's use std::ranges::sort and spaceship operator to sort with std::ranges::sort which is available on C++20.
Codecov Report
Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
Project coverage is 87.99%. Comparing base (
4cf6fab) to head (2a94e99). Report is 438 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/cleanup_queue-inl.h | 71.42% | 1 Missing and 1 partial :warning: |
| src/cleanup_queue.cc | 75.00% | 0 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #56063 +/- ##
==========================================
- Coverage 88.00% 87.99% -0.01%
==========================================
Files 656 656
Lines 188988 189002 +14
Branches 35992 35988 -4
==========================================
- Hits 166315 166310 -5
- Misses 15838 15849 +11
- Partials 6835 6843 +8
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| src/cleanup_queue.h | 100.00% <ΓΈ> (ΓΈ) |
|
| src/cleanup_queue.cc | 81.25% <75.00%> (-4.47%) |
:arrow_down: |
| src/cleanup_queue-inl.h | 85.00% <71.42%> (-7.31%) |
:arrow_down: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
cc @nodejs/build is there a limitation in macOS regarding this?
12:42:22 ../src/cleanup_queue-inl.h:35:17: error: no member named 'strong_ordering' in namespace 'std'
12:42:22 return std::strong_ordering::greater;
12:42:22 ~~~~~^
12:42:22 ../src/cleanup_queue-inl.h:39:17: error: no member named 'strong_ordering' in namespace 'std'
12:42:22 return std::strong_ordering::less;
12:42:22 ~~~~~^
12:42:22 ../src/cleanup_queue-inl.h:41:15: error: no member named 'strong_ordering' in namespace 'std'
12:42:22 return std::strong_ordering::equivalent;
12:42:22 ~~~~~^
Are you missing an
#include <compare>
Yes, but is it only required on macOS?
cc @nodejs/build This is blocked my macOS infrastructure as well.
20:44:45 ../src/cleanup_queue.cc:3:10: fatal error: 'ranges' file not found
20:44:45 #include <ranges>
20:44:45 ^~~~~~~~
20:44:46 1 error generated.
@anonrig do you know if this is still being blocked by macos test runners?
Yes it is still blocked
It's probably the opposite: I guess GCC was updated in the GitHub runners since the last push (two months ago) and includes new warnings.
It seems this broke the "Test Linux" action. Is a GCC update needed?
No the only blocker for this was the macOS build. So Linux shouldn't break unless something changed that's outside of the scope of this Pr in the past 2 months, that broke this pr when it landed.
@targos I am away from keyboard. Can you revert this PR? I can re-land it later. Right now your v8 work is more important.
FWIW the error is:
../src/cleanup_queue.cc:18:32: error: 'greater' may not intend to support class template argument deduction [-Werror,-Wctad-maybe-unsupported]
18 | std::ranges::sort(callbacks, std::greater());
| ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_function.h:390:12: note: add a deduction guide to suppress this warning
390 | struct greater : public binary_function<_Tp, _Tp, bool>
| ^
1 error generated.
The "Test Linux" workflow uses clang, not GCC. https://github.com/nodejs/node/blob/261624bb06f40b4fab410211148f8fdcfa33d209/.github/workflows/test-linux.yml#L28-L29
The "Test Linux" workflow uses clang, not GCC.
Although FWIW https://github.com/actions/runner-images/pull/11197 back in December updated GNU C++ from 13.2.0 to 13.3.0.
I am away from keyboard. Can you revert this PR? I can re-land it later. Right now your v8 work is more important.
https://github.com/nodejs/node/pull/56846