+1 — that feature as saved us from nasty issues already when working on internal modules.
> On Jul 9, 2018, at 8:43 AM, Zhitao Li <zhitaoli...@gmail.com> wrote: > > +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
