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

Make methods of derived Workers final instead of virtual #598

Closed
vweevers opened this issue Mar 1, 2019 · 2 comments
Closed

Make methods of derived Workers final instead of virtual #598

vweevers opened this issue Mar 1, 2019 · 2 comments
Labels
refactor Requires or pertains to refactoring

Comments

@vweevers
Copy link
Member

vweevers commented Mar 1, 2019

E.g. instead of:

leveldown/binding.cc

Lines 848 to 850 in 3ff48f1

virtual void DoExecute () {
SetStatus(database_->Put(options_, key_, value_));
}

Write:

 void DoExecute () final { 
   SetStatus(database_->Put(options_, key_, value_)); 
 }
@vweevers vweevers added the refactor Requires or pertains to refactoring label Mar 1, 2019
@ralphtheninja
Copy link
Member

Iirc they are still virtual so this is why I kept the virtual keyword. But fine if you want to remove it.

@vweevers
Copy link
Member Author

vweevers commented Mar 9, 2019

After reading a bit, I see why you'd want to keep virtual. It makes no difference to the compiler, but it's helpful information to humans. Without it you'd have to look at the base class to find a method is virtual.

So instead I want to:

  1. Make structs final (rather than their methods); I now realize that's what I wanted to communicate
  2. Replace virtual with the override keyword where appropriate (this still communicates the virtualness, and checks that the base class' method is virtual).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Requires or pertains to refactoring
Projects
None yet
Development

No branches or pull requests

2 participants