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.

Sign up for GitHub

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

Jump to bottom

src: use unique_ptr in platform implementation #16970

Closed
wants to merge 1 commit into from
Closed

src: use unique_ptr in platform implementation #16970

wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 12, 2017 •
edited
Loading

Replace raw pointers in task queues with std::unique_ptr. This
makes ownership clearer.

The future V8 platform interface uses smart pointers, so I figured we could change
the implementation proactively (and replace more raw pointers in other commits). What do you think?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 12, 2017
cjihrig
cjihrig approved these changes Nov 12, 2017
View reviewed changes
src/node_platform.cc Outdated
// In the long run, that functionality is probably going to be available
// anyway, though.
// Right now, there is no way to drain only background tasks associated
// with a specific isolate, so this sometimes does more work than
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space after "with"

@fhinkel
src: use unique_ptr in platform implementation
edf55ea 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.
@fhinkel
Copy link
Member Author

fhinkel commented Nov 13, 2017

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

@fhinkel
Copy link
Member Author

fhinkel commented Nov 14, 2017

VI failures look unrelated.

Thanks for the reviews. Landed in 8101042.

@fhinkel fhinkel closed this Nov 14, 2017
fhinkel added a commit to fhinkel/node that referenced this pull request Nov 14, 2017
@fhinkel
src: use unique_ptr in platform implementation
8101042 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

PR-URL: nodejs#16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@fhinkel fhinkel mentioned this pull request Nov 15, 2017
Closed
2 tasks
fhinkel added a commit that referenced this pull request Nov 17, 2017
@fhinkel
doc: mention smart pointers in Cpp style guide
0e1abb1 
Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr,
to the Cpp style guide. Mostly copied from the Google style guide.

PR-URL: #17055
Ref: #16970
Ref: #16974
Ref: #17000
Ref: #17012
Ref: #17020
Ref: #17030
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
@fhinkel @MylesBorins
src: use unique_ptr in platform implementation
8522e24 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

PR-URL: #16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@fhinkel @MylesBorins
doc: mention smart pointers in Cpp style guide
03803ee 
Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr,
to the Cpp style guide. Mostly copied from the Google style guide.

PR-URL: #17055
Ref: #16970
Ref: #16974
Ref: #17000
Ref: #17012
Ref: #17020
Ref: #17030
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@fhinkel @gibfahn
doc: mention smart pointers in Cpp style guide
f30fcdb 
Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr,
to the Cpp style guide. Mostly copied from the Google style guide.

PR-URL: #17055
Ref: #16970
Ref: #16974
Ref: #17000
Ref: #17012
Ref: #17020
Ref: #17030
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@fhinkel @gibfahn
doc: mention smart pointers in Cpp style guide
706aab9 
Add rule for smart pointers, i.e., std::unique_ptr and std::shared_ptr,
to the Cpp style guide. Mostly copied from the Google style guide.

PR-URL: #17055
Ref: #16970
Ref: #16974
Ref: #17000
Ref: #17012
Ref: #17020
Ref: #17030
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request May 22, 2018
@fhinkel @addaleax
src: use unique_ptr in platform implementation
916fa14 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

PR-URL: nodejs#16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Closed
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@fhinkel @MylesBorins
src: use unique_ptr in platform implementation
becb2df 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

Backport-PR-URL: #20901
PR-URL: #16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@fhinkel @rvagg
src: use unique_ptr in platform implementation
2148b19 
Replace raw pointers in task queues with std::unique_ptr. This
makes ownership obvious.

Backport-PR-URL: #20901
PR-URL: #16970
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fhinkel @jasnell @addaleax @cjihrig @gireeshpunathil @nodejs-github-bot