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: apply clang-tidy rule modernize-make-unique #26493

Closed
wants to merge 1 commit into from
Closed

src: apply clang-tidy rule modernize-make-unique #26493

wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

Apply clang-tidy rule https://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html

And also in Item 21: Prefer std::make_unique and std::make_shared to direct use of new.

  • Compared to direct use of new, make functions eliminate source code duplication, improve exception safety, and, for std::make_shared and std::allocate_shared, generate code that’s smaller and faster.
  • Situations where use of make functions is inappropriate include the need to specify custom deleters and a desire to pass braced initializers.

cc @addaleax @refack @bnoordhuis

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

@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 Mar 7, 2019
addaleax
addaleax approved these changes Mar 7, 2019
View reviewed changes
src/node_file.cc Outdated Show resolved Hide resolved
refack
refack approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@refack refack 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.

RSLGTM % two nits

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
src/inspector/worker_inspector.cc Show resolved Hide resolved
bnoordhuis
bnoordhuis approved these changes Mar 7, 2019
View reviewed changes
src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
src/inspector/worker_inspector.cc Outdated Show resolved Hide resolved
src/inspector/worker_inspector.cc Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the clang-tidy-make-unique branch 2 times, most recently from 407a683 to 95e1df2 Compare March 7, 2019 16:12
@danbev
Copy link
Contributor

danbev commented Mar 8, 2019

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2019
@danbev
Copy link
Contributor

danbev commented Mar 12, 2019

@gengjiawen Would you be able to rebase this and fix the conflict reported? Thanks

@gengjiawen
Copy link
Member Author

@danbev I can fix this evening :)

@gengjiawen
Copy link
Member Author

gengjiawen commented Mar 12, 2019 •
edited
Loading

@danbev done. Can you also help to re-trigger #26496 Jenkins CI ? Thanks.

@danbev
Copy link
Contributor

danbev commented Mar 12, 2019

Re-based CI: https://ci.nodejs.org/job/node-test-pull-request/21461/

@gengjiawen
Copy link
Member Author

@danbev I forced push because travis-ci found a lint issue ? Do you need to re-trigger this again ? Sorry for this inconvenience.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 12, 2019 •
edited
Loading

Landed in 575e086 🎉

@BridgeAR BridgeAR closed this Mar 12, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@gengjiawen @BridgeAR
src: apply clang-tidy rule modernize-make-unique
575e086 
PR-URL: nodejs#26493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BridgeAR
Copy link
Member

This does not land cleanly on v11. It seems to rely on other commits (e.g. #26306) that should be backported first. Please open a manual backport for it or change the labels accordingly.

@gengjiawen
Copy link
Member Author

gengjiawen commented Mar 13, 2019 •
edited
Loading

I am not familiar with backport process, how should I do it ?

@refack

This comment has been minimized.

Sign in to view
@BridgeAR
Copy link
Member

If a PR requires manual backports, we should first identify what code it relies upon. There are other PRs that should be backported first as the one that I pointed out. Otherwise there are more conflicts and they become worse over time.

So this one should likely not be the first to be backported. @joyeecheung recently added a tool to node-core-utils that is able to identify them (see https://asciinema.org/a/221244).

A backport itself is just a PR which targets the staging branches of the release line. In this case v11.x-staging instead of master.

@refack refack mentioned this pull request Mar 14, 2019
Closed
2 tasks
@refack
Copy link
Contributor

refack commented Mar 14, 2019

I opened a backport PR for this: #26651

@refack refack self-assigned this Mar 14, 2019
refack pushed a commit to refack/node that referenced this pull request Mar 14, 2019
@gengjiawen @refack
src: apply clang-tidy rule modernize-make-unique
9b73f1d 
PR-URL: nodejs#26493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Mar 28, 2019
@gengjiawen @targos
src: apply clang-tidy rule modernize-make-unique
2a1ce39 
PR-URL: #26493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
@gengjiawen @targos
src: apply clang-tidy rule modernize-make-unique
c51cc9e 
PR-URL: #26493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
@refack refack removed their assignment Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack 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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gengjiawen @danbev @BridgeAR @refack @bnoordhuis @addaleax @targos @nodejs-github-bot