mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: RFC: update C++ style to require the "override" keyword
Date Mon, 09 Jul 2018 22:38:10 GMT
+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.cs@gmail.com> wrote:
>> 
>> +1
>> 
>> On Sun, Jul 8, 2018 at 10:55 PM Benjamin Bannier <
>> benjamin.bannier@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

Mime
View raw message