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: make deleted functions public in node.h #25764

Closed
wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

Expert from Effective Modern C++ Items 11: Prefer deleted functions to private undefined ones

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.

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

src/node.h Outdated Show resolved Hide resolved
Signed-off-by: gengjiawen <technicalcute@gmail.com>
@addaleax
Copy link
Member

@danbev
Copy link
Contributor

danbev commented Jan 31, 2019

Landed in 0dfd5a5.

@danbev danbev closed this Jan 31, 2019
danbev pushed a commit that referenced this pull request Jan 31, 2019
Signed-off-by: gengjiawen <technicalcute@gmail.com>

PR-URL: #25764
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 1, 2019
Signed-off-by: gengjiawen <technicalcute@gmail.com>

PR-URL: #25764
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants