src: modernize cleanup queue to use c++20#56063

anonrig · 2024-11-28T18:46:25Z

Getting the idea from @jasnell on #56059, let's use std::ranges::sort and spaceship operator to sort with std::ranges::sort which is available on C++20.

codecov · 2024-11-28T20:16:50Z

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 ⚠️
src/cleanup_queue.cc	75.00%	0 Missing and 1 partial ⚠️

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%)`	⬇️
src/cleanup_queue-inl.h	`85.00% <71.42%> (-7.31%)`	⬇️

... and 27 files with indirect coverage changes

legendecas

LGTM, thanks! Commented some style thoughts...

src/cleanup_queue.cc

src/cleanup_queue.h

src/cleanup_queue.cc

src/cleanup_queue.h

nodejs-github-bot · 2024-11-29T17:39:12Z

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

anonrig · 2024-11-29T20:14:14Z

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 ~~~~~^

jasnell · 2024-11-29T20:56:24Z

Are you missing an #include <compare>

anonrig · 2024-11-29T21:35:13Z

Are you missing an #include <compare>

Yes, but is it only required on macOS?

anonrig · 2024-12-02T14:57:01Z

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.

jasnell · 2025-01-22T19:07:04Z

@anonrig do you know if this is still being blocked by macos test runners?

anonrig · 2025-01-23T00:49:21Z

@anonrig do you know if this is still being blocked by macos test runners?

Yes it is still blocked

jasnell · 2025-01-23T01:05:02Z

@nodejs/platform-macos

mcollina

lgtm

nodejs-github-bot · 2025-01-30T15:39:07Z

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

nodejs-github-bot · 2025-01-31T01:08:52Z

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

nodejs-github-bot · 2025-01-31T13:27:35Z

Landed in 581b444

lpinca · 2025-01-31T14:19:19Z

It seems this broke the "Test Linux" workflow. Is a GCC update needed?

targos · 2025-01-31T14:20:59Z

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.

anonrig · 2025-01-31T14:21:21Z

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.

anonrig · 2025-01-31T14:22:30Z

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

lpinca · 2025-01-31T14:26:04Z

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.

richardlau · 2025-01-31T14:34:08Z

The "Test Linux" workflow uses clang, not GCC.

node/.github/workflows/test-linux.yml

Lines 28 to 29 in 261624b

CC: sccache clang
CXX: sccache clang++

richardlau · 2025-01-31T14:38:39Z

The "Test Linux" workflow uses clang, not GCC.

Although FWIW actions/runner-images#11197 back in December updated GNU C++ from 13.2.0 to 13.3.0.

richardlau · 2025-01-31T15:21:24Z

I am away from keyboard. Can you revert this PR? I can re-land it later. Right now your v8 work is more important.

#56846

This reverts commit 581b444. PR-URL: #56846 Refs: #56063 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

anonrig added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Nov 28, 2024

anonrig requested review from jasnell, legendecas and tniessen November 28, 2024 18:46

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 28, 2024

anonrig requested review from RafaelGSS and mcollina November 29, 2024 00:32

legendecas reviewed Nov 29, 2024
View reviewed changes

src/cleanup_queue.cc Outdated Show resolvedHide resolved
src/cleanup_queue.h Outdated Show resolvedHide resolved

legendecas reviewed Nov 29, 2024
View reviewed changes

src/cleanup_queue.ccShow resolvedHide resolved

jasnell reviewed Nov 29, 2024
View reviewed changes

src/cleanup_queue.h Outdated Show resolvedHide resolved

anonrig force-pushed the yagiz/simplify-cleanup-queue branch 2 times, most recently from 440000e to 5cb91ddCompare November 29, 2024 16:43

anonrig requested review from jasnell and legendecas November 29, 2024 16:45

jasnell reviewed Nov 29, 2024
View reviewed changes

src/cleanup_queue.h Outdated Show resolvedHide resolved

jasnell approved these changes Nov 29, 2024
View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 29, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024

anonrig added the blocked PRs that are blocked by other issues or PRs. label Nov 29, 2024

anonrig force-pushed the yagiz/simplify-cleanup-queue branch from 5cb91dd to 23b6994Compare November 29, 2024 22:19

anonrig removed the blocked PRs that are blocked by other issues or PRs. label Nov 29, 2024

RafaelGSS approved these changes Dec 1, 2024
View reviewed changes

legendecas approved these changes Dec 1, 2024
View reviewed changes

anonrig added the blocked PRs that are blocked by other issues or PRs. label Dec 2, 2024

anonrig mentioned this pull request Dec 11, 2024
Infrastructure for Orka (2024 and beyond) nodejs/build#3686
Closed
12 tasks

mcollina approved these changes Jan 23, 2025
View reviewed changes

targos added commit-queue Add this label to land a pull request using GitHub Actions. and removed blocked PRs that are blocked by other issues or PRs. labels Jan 31, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 31, 2025

nodejs-github-bot merged commit 581b444 into nodejs:mainJan 31, 2025
59 checks passed

targos mentioned this pull request Jan 31, 2025
src: use std::ranges::greater instead of std::greater#56844
Closed

This was referenced Jan 31, 2025
tools: fix release URL computation in update-root-certs.mjs #56843
Merged
Revert "src: modernize cleanup queue to use c++20" #56846
Merged

aduh95 added the dont-land-on-v23.x label Feb 1, 2025

RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged

Uh oh!

src: modernize cleanup queue to use c++20#56063

src: modernize cleanup queue to use c++20 #56063

Uh oh!

Conversation

anonrig commented Nov 28, 2024

Uh oh!

codecovbot commented Nov 28, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

legendecas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 29, 2024

Uh oh!

anonrig commented Nov 29, 2024

Uh oh!

jasnell commented Nov 29, 2024

Uh oh!

anonrig commented Nov 29, 2024

Uh oh!

anonrig commented Dec 2, 2024

Uh oh!

jasnell commented Jan 22, 2025

Uh oh!

anonrig commented Jan 23, 2025

Uh oh!

jasnell commented Jan 23, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 30, 2025

Uh oh!

nodejs-github-bot commented Jan 31, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 31, 2025

Uh oh!

lpinca commented Jan 31, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 31, 2025

Uh oh!

anonrig commented Jan 31, 2025

Uh oh!

anonrig commented Jan 31, 2025

Uh oh!

lpinca commented Jan 31, 2025

Uh oh!

richardlau commented Jan 31, 2025

Uh oh!

richardlau commented Jan 31, 2025

Uh oh!

richardlau commented Jan 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecovbot commented Nov 28, 2024•
edited
Loading

lpinca commented Jan 31, 2025•
edited
Loading