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

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

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 28, 2024

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.

@anonrig anonrig added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in 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
@nodejs-github-bot 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
Copy link

codecov bot commented Nov 28, 2024

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

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Commented some style thoughts...

src/cleanup_queue.cc Outdated Show resolved Hide resolved
src/cleanup_queue.h Outdated Show resolved Hide resolved
src/cleanup_queue.h Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/simplify-cleanup-queue branch 2 times, most recently from 440000e to 5cb91dd Compare November 29, 2024 16:43
src/cleanup_queue.h Outdated Show resolved Hide resolved
@anonrig 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member Author

anonrig commented Nov 29, 2024

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

@anonrig anonrig added the blocked PRs that are blocked by other issues or PRs. label Nov 29, 2024
@jasnell
Copy link
Member

jasnell commented Nov 29, 2024

Are you missing an #include <compare>

@anonrig
Copy link
Member Author

anonrig commented Nov 29, 2024

Are you missing an #include <compare>

Yes, but is it only required on macOS?

@anonrig anonrig force-pushed the yagiz/simplify-cleanup-queue branch from 5cb91dd to 23b6994 Compare November 29, 2024 22:19
@anonrig anonrig removed the blocked PRs that are blocked by other issues or PRs. label Nov 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member Author

anonrig commented Dec 2, 2024

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 anonrig added the blocked PRs that are blocked by other issues or PRs. label Dec 2, 2024
@jasnell
Copy link
Member

jasnell commented Jan 22, 2025

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

@anonrig
Copy link
Member Author

anonrig commented Jan 23, 2025

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

Yes it is still blocked

@jasnell
Copy link
Member

jasnell commented Jan 23, 2025

@nodejs/platform-macos

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos 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 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 nodejs-github-bot merged commit 581b444 into nodejs:main Jan 31, 2025
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 581b444

@lpinca
Copy link
Member

lpinca commented Jan 31, 2025

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

@targos
Copy link
Member

targos commented Jan 31, 2025

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
Copy link
Member Author

anonrig commented Jan 31, 2025

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
Copy link
Member Author

anonrig commented Jan 31, 2025

@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
Copy link
Member

lpinca commented Jan 31, 2025

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
Copy link
Member

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

CC: sccache clang
CXX: sccache clang++

@richardlau
Copy link
Member

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
Copy link
Member

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

nodejs-github-bot pushed a commit that referenced this pull request Feb 1, 2025
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]>
@aduh95 aduh95 added the dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. label Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in 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. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants