Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

src: use smart pointer instead of new and delete #17020

Closed

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 14, 2017

Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.

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

stc

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 14, 2017
@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 14, 2017
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

May be just me of course, but I feel the old code is more explicit in what it's trying to do ("free the memory"), while the same behavior is now implicit due to unique_ptr going out of scope.

@fhinkel fhinkel force-pushed the nov/unique_ptr_instead_immidiate_delete branch from 8449b49 to 60b14f3 Compare November 14, 2017 21:47
@Fishrock123
Copy link
Contributor

agree with @TimothyGu

Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.
@fhinkel fhinkel force-pushed the nov/unique_ptr_instead_immidiate_delete branch from 60b14f3 to 43635c0 Compare November 15, 2017 23:31
@fhinkel
Copy link
Member Author

fhinkel commented Nov 15, 2017

@Fishrock123, @TimothyGu I understand your concern. I added a comment to make the delete more explicit. I think as we move to smart pointers we shouldn't leave those as raw pointers.

@fhinkel
Copy link
Member Author

fhinkel commented Nov 16, 2017

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Still not a big fan, but LGTM if we were to completely eliminate dumb pointers from the code base.

@fhinkel
Copy link
Member Author

fhinkel commented Nov 17, 2017

Landed in f96abea.

@fhinkel fhinkel closed this Nov 17, 2017
fhinkel added a commit to fhinkel/node that referenced this pull request Nov 17, 2017
Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.

PR-URL: nodejs#17020
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
fhinkel added a commit that referenced this pull request Nov 17, 2017
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 12, 2017
Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.

PR-URL: #17020
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
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
@MylesBorins
Copy link
Contributor

This will need a manual backport for v6.x

gibfahn pushed a commit that referenced this pull request Dec 19, 2017
Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.

PR-URL: #17020
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
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 gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Use an std::unique_ptr for variables that are deleted
right after creation.

Since the destructor of InspectorTimer is private
but needed by the unique_ptr, define deleter_type as friend.

PR-URL: #17020
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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 gibfahn mentioned this pull request Dec 20, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants