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

src: apply clang-tidy rule modernize-make-unique by gengjiawen · Pull Request #26493 · nodejs/node · GitHub

/ node Public

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

Closed
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:clang-tidy-make-unique
ConversationCommits (1)ChecksFiles changed
Closed

src: apply clang-tidy rule modernize-make-unique#26493
gengjiawen wants to merge 1 commit into
nodejs:masterfrom
gengjiawen:clang-tidy-make-unique

Conversation

gengjiawen commented Mar 7, 2019

Copy link
Copy Markdown
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 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 approved these changes Mar 7, 2019
View reviewed changes
Comment thread src/node_file.cc Outdated
gengjiawen force-pushed the clang-tidy-make-unique branch from dcbea48 to b4a8d0f Compare March 7, 2019 13:47
refack approved these changes Mar 7, 2019
View reviewed changes

refack left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

RSLGTM % two nits

Comment thread src/inspector/main_thread_interface.cc Outdated
Comment thread src/inspector/worker_inspector.cc Outdated
gengjiawen force-pushed the clang-tidy-make-unique branch from b4a8d0f to aa2e90c Compare March 7, 2019 14:21
bnoordhuis approved these changes Mar 7, 2019
View reviewed changes
Comment thread src/inspector/main_thread_interface.cc Outdated
Comment thread src/inspector/worker_inspector.cc Outdated
Comment thread src/inspector/worker_inspector.cc Outdated
gengjiawen force-pushed the clang-tidy-make-unique branch 2 times, most recently from 407a683 to 95e1df2 Compare March 7, 2019 16:12

danbev commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 8, 2019

danbev commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

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

gengjiawen commented Mar 12, 2019

Copy link
Copy Markdown
Member Author

@danbev I can fix this evening :)

gengjiawen force-pushed the clang-tidy-make-unique branch from 95e1df2 to c04401d Compare March 12, 2019 14:28

gengjiawen commented Mar 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

danbev commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

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

gengjiawen force-pushed the clang-tidy-make-unique branch from c04401d to 8eefed0 Compare March 12, 2019 15:14

gengjiawen commented Mar 12, 2019

Copy link
Copy Markdown
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 commented Mar 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Landed in 575e086 🎉

BridgeAR closed this Mar 12, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
src: apply clang-tidy rule modernize-make-unique
575e086 
PR-URL: nodejs#26493
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

BridgeAR commented Mar 13, 2019

Copy link
Copy Markdown
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 commented Mar 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

This comment has been minimized.

Sign in to view

BridgeAR commented Mar 13, 2019

Copy link
Copy Markdown
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 mentioned this pull request Mar 14, 2019
Closed
2 tasks

refack commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

I opened a backport PR for this: #26651

refack self-assigned this Mar 14, 2019
refack pushed a commit to refack/node that referenced this pull request Mar 14, 2019
src: apply clang-tidy rule modernize-make-unique
9b73f1d 
PR-URL: nodejs#26493
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Mar 28, 2019
src: apply clang-tidy rule modernize-make-unique
2a1ce39 
PR-URL: #26493
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Mar 30, 2019
src: apply clang-tidy rule modernize-make-unique
c51cc9e 
PR-URL: #26493
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs mentioned this pull request Apr 9, 2019
Merged
refack removed their assignment Apr 14, 2019
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
addaleax addaleax approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

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++. 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

Back | FazBrowse Home | New Git URL