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.

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

src: inline macro DISALLOW_COPY_AND_ASSIGN #26634

Merged
merged 2 commits into from
Mar 18, 2019

Conversation

gengjiawen
Copy link
Member

@gengjiawen gengjiawen commented Mar 13, 2019

This one is similar like #25764, following Effective Modern C++ Items 11: Prefer deleted functions to private undefined ones.

This time I tried this on godbolt.
When deleted functions are declared private,
GCC 6.2 error message is not very clear.
GCC 8.x and Clang 7.x error message is clear.
demo: https://godbolt.org/z/k6O9wc.

This commit also add commit use deleted function instead of private function in class AsyncWrap

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 13, 2019
@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch from b1c31b5 to 8198e3b Compare March 13, 2019 15:10
@gengjiawen
Copy link
Member Author

Linting failed due to

src/node_buffer.cc:91:  DISALLOW_COPY_AND_ASSIGN must be in the private: section  [readability/constructors] [3]
src/node_buffer.cc:91:  DISALLOW_COPY_AND_ASSIGN should be the last thing in the class  [readability/constructors] [3]

cc @addaleax @refack @bnoordhuis

@refack
Copy link
Contributor

refack commented Mar 13, 2019

DISALLOW_COPY_AND_ASSIGN must be in the private: section [readability/constructors]

That's strange since it's contrary to https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types:

Decision:

Every class's public interface should make explicit which copy and move operations the class supports. This should usually take the form of explicitly declaring and/or deleting the appropriate operations in the public section of the declaration.

I'd be +1 for unpacking the macro everywhere. Personally I'm ambivalent of it's use... On the one hand it has an nice explicit name. On the other hand it's non-language meta-programming (ES.30: Don’t use macros for program text manipulation & https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros)

I'll post a PR to patch cpplint.py.

@gengjiawen
Copy link
Member Author

Personally I like to unpack them too.

@refack
Copy link
Contributor

refack commented Mar 13, 2019

following Effective Modern C++ [Items 11: Prefer deleted functions to private undefined ones](http://clchiou.github.io/notes-effective-modern-c++/2017-01-14/item-11-prefer-deleted-functions-to-private-undefined-ones).

FTR: the reason behind this is Deleted functions should be declared public because compiler checks accessibility before deleted status. So deleting default operation in public will give a better compilation error ("x was deleted" rather then "x is private")

@gengjiawen
Copy link
Member Author

gengjiawen commented Mar 13, 2019

Yeap, the pr I refered has the similar information.

By convention, deleted functions are declared public, not private. There’s a reason for that. When client code tries to use a member function, C++ checks accessibility before deleted status. When client code tries to use a deleted private function, some compilers complain only about the function being private, even though the function’s accessibility doesn’t really affect whether it can be used. It’s worth bearing this in mind when revising legacy code to replace private-and-not-defined member functions with deleted ones, because making the new functions public will generally result in better error messages.

@gengjiawen gengjiawen changed the title src: make deleted function public using macro DISALLOW_COPY_AND_ASSIGN src: make deleted function public when using macro DISALLOW_COPY_AND_ASSIGN Mar 13, 2019
@gengjiawen
Copy link
Member Author

I am thinking inline DISALLOW_COPY_AND_ASSIGN more be more appropriate, thoughts ?

cc @addaleax @bnoordhuis @jasnell @joyeecheung

@addaleax
Copy link
Member

I don’t have a strong opinion on this, personally.

@gengjiawen
Copy link
Member Author

I don’t have a strong opinion on this, personally.

If there is no strong objection on this, I can work on this on Sunday.

@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch from 8198e3b to 46f5862 Compare March 16, 2019 15:46
@gengjiawen gengjiawen changed the title src: make deleted function public when using macro DISALLOW_COPY_AND_ASSIGN src: inline macro DISALLOW_COPY_AND_ASSIGN Mar 16, 2019
@gengjiawen
Copy link
Member Author

@addaleax @refack I have inlined the macro and make it in public. Can you review this, thanks.

@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch 2 times, most recently from d21c189 to c6dd193 Compare March 16, 2019 15:51
src/node_file.h Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch 3 times, most recently from 54bfa35 to 6e9c44c Compare March 16, 2019 15:56
@refack
Copy link
Contributor

refack commented Mar 16, 2019

Non blocking, but it would be nice to update the example in

// private:
// ~MyData() override {}
// DISALLOW_COPY_AND_ASSIGN(MyData);

@refack
Copy link
Contributor

refack commented Mar 16, 2019

@refack
Copy link
Contributor

refack commented Mar 16, 2019

@gengjiawen
Copy link
Member Author

Non blocking, but it would be nice to update the example in

node/src/tracing/trace_event_common.h

Lines 153 to 155 in 6608cf2

// private:
// ~MyData() override {}
// DISALLOW_COPY_AND_ASSIGN(MyData);

Done.

@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch from 156e239 to 138d76c Compare March 17, 2019 02:31
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
@gengjiawen gengjiawen force-pushed the deleted_function_public_in_macro branch from 138d76c to 45d9454 Compare March 17, 2019 13:16
@refack
Copy link
Contributor

refack commented Mar 17, 2019

@refack
Copy link
Contributor

refack commented Mar 17, 2019

PR-URL: nodejs#26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: nodejs#26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack force-pushed the deleted_function_public_in_macro branch from 45d9454 to dace489 Compare March 18, 2019 01:59
@refack
Copy link
Contributor

refack commented Mar 18, 2019

Quick pre-land sanity: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2953/

@refack refack merged commit dace489 into nodejs:master Mar 18, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26634
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants