+1 On Mon, Jul 9, 2018 at 3:38 PM, Andrew Schwartzmeyer < and...@schwartzmeyer.com> wrote:
> +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 >>> >>
