+1 On Sun, Jul 8, 2018 at 10:55 PM Benjamin Bannier < benjamin.bann...@mesosphere.io> wrote:
> 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 > > -- Cheers, Zhitao Li
