Skip to content

src: remove usage of std::shared_ptr<T>::unique()#47315

Closed
RaisinTen wants to merge 1 commit intonodejs:mainfrom
RaisinTen:src/remove-usage-of-std-shared_ptr-unique
Closed

src: remove usage of std::shared_ptr<T>::unique()#47315
RaisinTen wants to merge 1 commit intonodejs:mainfrom
RaisinTen:src/remove-usage-of-std-shared_ptr-unique

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Mar 30, 2023

std::shared_ptr<T>::unique() has been removed in C++20, so this change uses std::shared_ptr<T>::use_count() instead which is available in C++20.

Fixes: #47311

@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 Mar 30, 2023
@RaisinTen RaisinTen mentioned this pull request Mar 30, 2023
Closed
@RaisinTen
src: remove usage of std::shared_ptr<T>::unique()
409158a 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: nodejs#47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the src/remove-usage-of-std-shared_ptr-unique branch from 7ea5ed1 to 409158a Compare March 30, 2023 10:19
tniessen
tniessen approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Based on the name, I assume that this particular function is not supposed to be thread-safe.

@RaisinTen
Copy link
Member Author

RaisinTen commented Mar 30, 2023

Correct, there's also a comment saying that -

node/src/node_threadsafe_cow.h

Lines 14 to 17 in 65b79ab

// Copy-on-write utility. Not threadsafe, i.e. there is no synchronization
// of the copy operation with other operations.
template <typename T>
class CopyOnWrite final {

@RaisinTen RaisinTen 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 Mar 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Mar 31, 2023
Open
34 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
bnoordhuis
bnoordhuis approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The non-functional change LGTM but the code itself has a race condition, hasn't it?

edit: I get that it's protected by a mutex but the way that file is structured seems very prone to future mistakes.

This was referenced Apr 1, 2023
Open
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Apr 4, 2023
Open
29 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 4, 2023

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

@RaisinTen
Copy link
Member Author

RaisinTen commented Apr 4, 2023

I don't know why the GitHub UI is showing ${STATUS_LABEL} as a failing Jenkins job. I think it's an outdated one because it shows Mar 30, 2023, 6:35:48 AM as the time but the last CI job was run on Apr 4, 2023, 1:25:14 AM. This will probably need to be landed manually.

@github-actions github-actions bot mentioned this pull request Apr 5, 2023
Open
26 tasks
RaisinTen added a commit that referenced this pull request Apr 5, 2023
@RaisinTen
src: remove usage of std::shared_ptr<T>::unique()
1ff9824 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RaisinTen
Copy link
Member Author

RaisinTen commented Apr 5, 2023

Landed in 1ff9824

@RaisinTen RaisinTen closed this Apr 5, 2023
@RaisinTen RaisinTen deleted the src/remove-usage-of-std-shared_ptr-unique branch April 5, 2023 13:09
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
d3d050e 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
26a199b 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
4a3ec94 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
02cce66 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
0695151 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
@RaisinTen @RafaelGSS
src: remove usage of std::shared_ptr<T>::unique()
98ece91 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@RaisinTen @danielleadams
src: remove usage of std::shared_ptr<T>::unique()
f7b6e5b 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: #47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@RaisinTen @MoLow
src: remove usage of std::shared_ptr<T>::unique()
ded4a5e 
`std::shared_ptr<T>::unique()` has been removed in C++20, so this change
uses `std::shared_ptr<T>::use_count()` instead which is available in
C++20.

Fixes: nodejs#47311
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#47315
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

Assignees

No one assigned

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++. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

std::shared_ptr<T>::unique was removed in C++20

8 participants

@RaisinTen @nodejs-github-bot @bnoordhuis @jasnell @anonrig @cjihrig @tniessen @gengjiawen