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

Jump to bottom

util: update definition of DISALLOW_COPY_AND_ASSIGN macro #23092

Closed
wants to merge 1 commit into from
Closed

util: update definition of DISALLOW_COPY_AND_ASSIGN macro #23092

wants to merge 1 commit into from

Conversation

nitsakh
Copy link
Contributor

@nitsakh nitsakh commented Sep 25, 2018

This PR updates the DISALLOW_COPY_AND_ASSIGN macro.
The compiler will not define move constructor or move assignment operators when copy constructor/assignment is defined.
Hence, it's not needed to delete move operations.
This also makes the definition coherent with chromium's definition of the same. https://cs.chromium.org/chromium/src/base/macros.h?rcl=b209442fa2c29021b06a6dcbbd0486b440011fe2&l=33.

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 25, 2018
bnoordhuis
bnoordhuis approved these changes Sep 26, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. You're right they're not needed. I believe I added them to work around bugs in VS 2013 and left them in afterwards as self-documentation.

@addaleax
Copy link
Member

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

@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 30, 2018
@danbev
Copy link
Contributor

danbev commented Oct 2, 2018

Re-run CI: https://ci.nodejs.org/job/node-test-pull-request/17577/

@joyeecheung joyeecheung mentioned this pull request Oct 2, 2018
Closed
@danbev
Copy link
Contributor

danbev commented Oct 2, 2018

Re-run failing node-test-commit-osx.

@danbev
Copy link
Contributor

danbev commented Oct 2, 2018

Landed in 97f1e94.

@danbev danbev closed this Oct 2, 2018
danbev pushed a commit that referenced this pull request Oct 2, 2018
@nitsakh @danbev
util: update definition of DISALLOW_COPY_AND_ASSIGN macro
97f1e94 
PR-URL: #23092
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@nitsakh @targos
util: update definition of DISALLOW_COPY_AND_ASSIGN macro
f2dfebb 
PR-URL: #23092
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nitsakh nitsakh deleted the copy-assign-macro branch October 4, 2018 05:43
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@addaleax addaleax mentioned this pull request Oct 23, 2018
Merged
2 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell 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.
6 participants
@nitsakh @addaleax @danbev @bnoordhuis @jasnell @nodejs-github-bot