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.

Sign up for GitHub

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

Jump to bottom

src: add override to ThreadPool methods in zlib #20769

Closed
wants to merge 2 commits into from
Closed

src: add override to ThreadPool methods in zlib #20769

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 16, 2018

Currently the following compiler warnings are generated:

../src/node_zlib.cc:222:8:
warning: 'DoThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void DoThreadPoolWork() {
       ^
../src/node_internals.h:509:16: note: overridden virtual function is here
  virtual void DoThreadPoolWork() = 0;
               ^
../src/node_zlib.cc:357:8: warning:
'AfterThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void AfterThreadPoolWork(int status) {
       ^
../src/node_internals.h:510:16:
note: overridden virtual function is here
  virtual void AfterThreadPoolWork(int status) = 0;
               ^

This commit adds the override specifier to the methods to silence the
warnings.

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

@danbev
src: add override to ThreadPool methods in zlib
98b4faa 
Currently the following compiler warnings are generated:
../src/node_zlib.cc:222:8:
warning: 'DoThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void DoThreadPoolWork() {
       ^
../src/node_internals.h:509:16: note: overridden virtual function is here
  virtual void DoThreadPoolWork() = 0;
               ^
../src/node_zlib.cc:357:8: warning:
'AfterThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void AfterThreadPoolWork(int status) {
       ^
../src/node_internals.h:510:16:
note: overridden virtual function is here
  virtual void AfterThreadPoolWork(int status) = 0;
               ^

This commit adds the override specifier to the methods to silence the
warnings.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels May 16, 2018
@danbev
Copy link
Contributor Author

danbev commented May 16, 2018

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

addaleax
addaleax approved these changes May 16, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Not sure if my compiler just doesn’t display these or I’m overlooking them 🙃

@bnoordhuis
Copy link
Member

@addaleax You use gcc < 5? -Winconsistent-missing-override is a clang special. gcc calls it -Wsuggest-override and (I think) turns it on with -Wall.

@addaleax
Copy link
Member

7.2.0, actually. It does provide warnings when used with -Wsuggest-override, but that suggests override for other spots in the source too.

@danbev danbev added the fast-track PRs that do not need to wait for 48 hours to land. label May 17, 2018
@danbev
Copy link
Contributor Author

danbev commented May 17, 2018

Would it be alright to fast-track this issue?

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018
@danbev
Copy link
Contributor Author

danbev commented May 18, 2018

Landed in 0419adc.

@danbev danbev closed this May 18, 2018
@danbev danbev deleted the zlib_compiler_warning branch May 18, 2018 05:13
danbev added a commit that referenced this pull request May 18, 2018
@danbev
src: add override to ThreadPool methods in zlib
0419adc 
Currently the following compiler warnings are generated:
../src/node_zlib.cc:222:8:
warning: 'DoThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void DoThreadPoolWork() {
       ^
../src/node_internals.h:509:16: note: overridden virtual function is here
  virtual void DoThreadPoolWork() = 0;
               ^
../src/node_zlib.cc:357:8: warning:
'AfterThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void AfterThreadPoolWork(int status) {
       ^
../src/node_internals.h:510:16:
note: overridden virtual function is here
  virtual void AfterThreadPoolWork(int status) = 0;
               ^

This commit adds the override specifier to the methods to silence the
warnings.

PR-URL: #20769
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@danbev @MylesBorins
src: add override to ThreadPool methods in zlib
d6805c1 
Currently the following compiler warnings are generated:
../src/node_zlib.cc:222:8:
warning: 'DoThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void DoThreadPoolWork() {
       ^
../src/node_internals.h:509:16: note: overridden virtual function is here
  virtual void DoThreadPoolWork() = 0;
               ^
../src/node_zlib.cc:357:8: warning:
'AfterThreadPoolWork' overrides a member function but is not marked
      'override' [-Winconsistent-missing-override]
  void AfterThreadPoolWork(int status) {
       ^
../src/node_internals.h:510:16:
note: overridden virtual function is here
  virtual void AfterThreadPoolWork(int status) = 0;
               ^

This commit adds the override specifier to the methods to silence the
warnings.

PR-URL: #20769
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis 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++. fast-track PRs that do not need to wait for 48 hours to land. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@danbev @bnoordhuis @addaleax @jasnell @cjihrig @joyeecheung @lpinca @nodejs-github-bot