src: do not redefine private for GenDebugSymbols by joyeecheung · Pull Request #18653 · nodejs/node

joyeecheung · 2018-02-08T19:38:22Z

Redefining private breaks any private inheritance in the
included files. We can simply declare GenDebugSymbols()
as friends in related classes to gain the access that we need.

Refs: #14901 (comment)

Checklist

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

Affected core subsystem(s)

src

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need.

joyeecheung · 2018-02-08T19:39:55Z

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

mmarchini

Redefining private was a reasonable approach for the first version of GenDebugSymbols when it was created by a Python script, but for the current approach explicitly declaring those classes as friends of GenDebugSymbols makes more sense IMO.

mmarchini · 2018-02-08T20:22:37Z

src/node_postmortem_metadata.cc

@@ -44,14 +44,6 @@
 #include <utility>


We can remove those includes as well since they are only here to avoid problems with private class inheritance

joyeecheung · 2018-02-08T20:34:30Z

@mmarchini Thanks for the suggestion, updated.

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

bnoordhuis · 2018-02-08T21:10:13Z

src/base_object.h


 namespace node {

+int GenDebugSymbols();


Correct me if I'm wrong but I don't think you have to forward-declare it, just the friend stanza inside the class is enough.

joyeecheung · 2018-02-08T21:57:05Z

@bnoordhuis Indeed, those forward declarations are not necessary. Thanks!

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

joyeecheung · 2018-02-14T05:31:32Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/13167/

joyeecheung · 2018-02-14T08:54:35Z

There were some infra issues. New CI is green: https://ci.nodejs.org/job/node-test-commit/16238/

joyeecheung · 2018-02-14T08:55:52Z

Landed in 18d23aa, thanks!

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-02-21T14:30:10Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

joyeecheung · 2018-02-21T18:24:25Z

This is a fix for #14901 so cannot land until that one lands.

mmarchini · 2018-02-22T15:44:22Z

v9.x backport PR: #18550

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: nodejs#18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #18550 PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: nodejs#18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need. Backport-PR-URL: #19176 PR-URL: #18653 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

src: do not redefine private for GenDebugSymbols

cce09c3

Redefining private breaks any private inheritance in the included files. We can simply declare GenDebugSymbols() as friends in related classes to gain the access that we need.

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 Feb 8, 2018

addaleax approved these changes Feb 8, 2018

View reviewed changes

mmarchini approved these changes Feb 8, 2018

View reviewed changes

squash! remove unnecessary std lib includes

b56fc10

bnoordhuis approved these changes Feb 8, 2018

View reviewed changes

squash! remove unnecessary forward declaration

b37013c

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2018

cjihrig approved these changes Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

joyeecheung closed this Feb 14, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2018

mmarchini mentioned this pull request Feb 22, 2018

[v9.x backport] node internals' postmortem metadata (#14901, #18530, #18653, #18576) #18550

Closed

4 tasks

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

mmarchini mentioned this pull request Mar 6, 2018

[v8.x backport] node internals' postmortem metadata (#14901, #18530, #18653) #19176

Closed

4 tasks

mmarchini mentioned this pull request Mar 13, 2018

[v6.x backport] node internals' postmortem metadata (#14901, #18530, #18653) #19323

Closed

4 tasks

MylesBorins added the backported-to-v8.x label May 22, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

This was referenced Mar 7, 2020

[Snyk] Security upgrade gatsby from 2.13.73 to 2.16.2 meghasfdc/node#4

Open

[Snyk] Security upgrade gatsby from 2.13.73 to 2.16.2 kissofire/node#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: do not redefine private for GenDebugSymbols#18653

src: do not redefine private for GenDebugSymbols#18653
joyeecheung wants to merge 3 commits intonodejs:masterfrom
joyeecheung:remove-private-redefine

joyeecheung commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

mmarchini left a comment

Uh oh!

mmarchini Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

bnoordhuis Feb 8, 2018

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

MylesBorins commented Feb 21, 2018

Uh oh!

joyeecheung commented Feb 21, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

joyeecheung commented Feb 8, 2018

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

bnoordhuis Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 8, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

joyeecheung commented Feb 14, 2018

Uh oh!

MylesBorins commented Feb 21, 2018

Uh oh!

joyeecheung commented Feb 21, 2018

Uh oh!

mmarchini commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants