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

[Zhitao Li]

+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

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

[Benjamin Bannier]

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

Behavior change for the cgroups mounts inside container

[Qian Zhang]

Hi Folks,

Recently we did a behavior change for the cgroups mounts inside the
containers launched by UCR in the ticket MESOS-8327
: For the container with
its own rootfs, before the change, it will see all cgroups as the agent
host, and after the change, we will do container specific cgroups mounts in
the container's mount namespace so it will only see its own cgroups which
is also the default behavior of Docker, see more details in the design doc

.

Please feel free to let us know for any comments, thanks!


Regards,
Qian Zhang

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

[James Peach]

Hi all,

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.

--- a/docs/c++-style-guide.md
+++ b/docs/c++-style-guide.md
@@ -647,3 +647,16 @@ Const expression constructors allow object initialization 
at compile time provid
   C++11 does not provide `constexpr string` or `constexpr` containers in the 
STL and hence `constexpr` cannot be used for any class using stout's Error() 
class.

 * `enum class`.
+
+* `override`.
+
+When overriding a virtual member function, the `override` keyword should 
always be used.  The [Google C++ Style 
Guide](https://google.github.io/styleguide/cppguide.html#Inheritance) supplies 
the rationale for this:
+
+
+A function or destructor marked override or final that is not an
+override of a base class virtual function will not compile, and
+this helps catch common errors. The specifiers serve as documentation;
+if no specifier is present, the reader has to check all ancestors
+of the class in question to determine if the function or destructor
+is virtual or not.
+

thanks,
James