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

Introduce 'override' and 'final' keywords following C++ Core Guidelines #600

Merged
merged 2 commits into from
Mar 11, 2019

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Mar 9, 2019

Closes #598 and implements rule C128 of the C++ Core Guidelines:

Virtual functions should specify exactly one of virtual, override, or final

Reason Readability. Detection of mistakes. Writing explicit virtual, override, or final is self-documenting and enables the compiler to catch mismatch of types and/or names between base and derived classes. However, writing more than one of these three is both redundant and a potential source of errors.
It’s simple and clear:

  • virtual means exactly and only “this is a new virtual function.”
  • override means exactly and only “this is a non-final overrider.”
  • final means exactly and only “this is a final overrider.”

If a base class destructor is declared virtual, one should avoid declaring derived class destructors virtual or override. Some code base and tools might insist on override for destructors, but that is not the recommendation of these guidelines.

@vweevers vweevers requested review from peakji and filoozom March 9, 2019 11:51
@peakji
Copy link
Member

peakji commented Mar 11, 2019

Since we're talking about readability and code style, I'd suggest to swap line L336 and L337 to stop the compiler from complaining about initialization order:

  CXX(target) Release/obj.target/leveldown/binding.o
../binding.cc:336:7: warning: field 'priorityWork_' will be initialized after field 'pendingCloseWorker_' [-Wreorder]
      priorityWork_(0),
      ^
1 warning generated.

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.

2 participants