FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: remove usage of `std::shared_ptr<T>::unique()` by RaisinTen · Pull Request #47315 · nodejs/node · GitHub

/ node Public

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

Closed
RaisinTen wants to merge 1 commit into
nodejs:mainfrom
RaisinTen:src/remove-usage-of-std-shared_ptr-unique
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

RaisinTen commented Mar 30, 2023

Copy link
Copy Markdown
Member

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 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 mentioned this pull request Mar 30, 2023
Closed
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 force-pushed the src/remove-usage-of-std-shared_ptr-unique branch from 7ea5ed1 to 409158a Compare March 30, 2023 10:19
tniessen approved these changes Mar 30, 2023
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

RaisinTen commented Mar 30, 2023

Copy link
Copy Markdown
Member Author

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 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 30, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2023

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

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

This comment was marked as outdated.

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

bnoordhuis left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view
github-actions Bot mentioned this pull request Apr 4, 2023
Open
29 tasks

nodejs-github-bot commented Apr 4, 2023

Copy link
Copy Markdown
Collaborator

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

RaisinTen commented Apr 4, 2023

Copy link
Copy Markdown
Member Author

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 Bot mentioned this pull request Apr 5, 2023
Open
26 tasks
RaisinTen added a commit that referenced this pull request Apr 5, 2023
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 commented Apr 5, 2023

Copy link
Copy Markdown
Member Author

Landed in 1ff9824

RaisinTen closed this Apr 5, 2023
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
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 mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
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
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
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
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
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
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 mentioned this pull request Jul 10, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
anonrig anonrig approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
gengjiawen gengjiawen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL