Hi James,
> I’d like to propose that we update our style to require that the
> “override” keyword always be used when overriding virtual functions
> (including destructors). The proposed text is below. I’ll also prepare
> a clang-tidy patch to update stout, libprocess and mesos globally.
+1!
Thanks for bringing this up and offering to do the clean-up. Using `override`
consistently would really give us some certainty as interface methods evolve.
* * *
Note that since our style guide _is_ the Google style guide plus some
additions, we shouldn't need to update anything in our style guide; the Google
style guide seems to have started requiring this from February this year and
our code base just got out of sync.
I believe we should activate the matching warning in our `cpplint` setup,
--- a/support/mesos-style.py
+++ b/support/mesos-style.py
@@ -256,6 +256,7 @@ class CppLinter(LinterBase):
'build/endif_comment',
'build/nullptr',
'readability/todo',
+ 'readability/inheritance',
'readability/namespace',
'runtime/vlog',
'whitespace/blank_line',
While e.g., `clang` already emits a diagnostic for hidden `virtual` functions
we might still want to update our `clang-tidy` setup. There is a dedicated
linter for `override` which me might not need due to the default diagnostic,
--- a/support/clang-tidy
+++ b/support/clang-tidy
@@ -25,6 +25,7 @@ google-*,\
mesos-*,\
\
misc-use-after-move,\
+modernize-use-override,\
\
readability-redundant-string-cstr\
"
but it probably makes a lot of sense to check what other compile-time Mesos
features can be enabled by default in our `clang-tidy` setup (either in Jenkins
via `CMAKE_ARGS`, or even better globally by default in
`support/mesos-tidy/entrypoint.sh:31ff`).
I would guess that using `cpplint` to verifying automated fixes made with
`clang-tidy` could inform what flags should have been added (there are some
missing features in the cmake build though, e.g., some isolators which would
have benefited from `override` recently).
Cheers,
Benjamin
