src: add missing virtual destructors #23215

danbev · 2018-10-02T11:02:46Z

This pull request contains three commit which add missing virtual destructors. Please see the individual commit messages for details.

Checklist

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

Currently the Options class has a virtual function but no virtual destructor which means that if delete is called on a Options pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on non-final 'node::PerIsolateOptions' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; This commit adds a virtual destructor.

Currently the KeyPairGenerationConfigs class has a virtual function but no virtual destructor which means that if delete is called on a KeyPairGenerationConfig pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on 'node::crypto::KeyPairGenerationConfig' that is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; This commit adds a virtual destructor.

Currently the WorkerDelegate class has a virtual function but no virtual destructor which means that if delete is called on a WorkerDelegate pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on 'node::inspector::WorkerDelegate' that is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; ^ This commit adds a virtual destructor.

nodejs-github-bot · 2018-10-02T11:02:47Z

@danbev build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1096/pipeline

danbev · 2018-10-02T11:03:11Z

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

benjamingr · 2018-10-02T13:50:19Z

Can this be something we lint for?

addaleax

@benjamingr The commit messages mention a compiler warning … I guess that might depend on the compiler version/flags?

benjamingr · 2018-10-02T14:22:56Z

Checked, I think cpplint can catch this ( https://stackoverflow.com/a/3906866/1348195 ) and so does gcc when building (and clang too probably).

It's just not a warning we have turned on by default probably or don't fail the build on - maybe that's the path forward.

I don't do much C++ for Node (yet!) - but in my own C++ I've found this exact issue to be quite insidious and to cause hard to debug bugs.

joyeecheung · 2018-10-02T15:00:09Z

@benjamingr Our cpplint.py doesn't seen to catch missing virtual constructors (only redundant virtual specifiers that come before an override/final)

(Also, our cpplint does not seem to be able to handle cross-file analysis at all..)

danbev · 2018-10-03T06:15:41Z

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

danbev · 2018-10-04T10:34:31Z

Re-build of failing node-test-commit-linux-containered.
Re-build of failing node-test-commit-arm.

danbev · 2018-10-05T03:18:20Z

Landed in e5c7f10, a8c1458, and 1043660.

Currently the Options class has a virtual function but no virtual destructor which means that if delete is called on a Options pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on non-final 'node::PerIsolateOptions' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; This commit adds a virtual destructor. PR-URL: #23215 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently the KeyPairGenerationConfigs class has a virtual function but no virtual destructor which means that if delete is called on a KeyPairGenerationConfig pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on 'node::crypto::KeyPairGenerationConfig' that is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; This commit adds a virtual destructor. PR-URL: #23215 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Currently the WorkerDelegate class has a virtual function but no virtual destructor which means that if delete is called on a WorkerDelegate pointer to a derived instance, the derived destructor will not get called. The following warning is currently being printed when compiling: warning: delete called on 'node::inspector::WorkerDelegate' that is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor] delete __ptr; ^ This commit adds a virtual destructor. PR-URL: #23215 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>