Re: RFC: update C++ style to require the "override" keyword

[Andrew Schwartzmeyer]

+1

On 07/09/2018 8:32 am, Till Toenshoff wrote:

+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