Re: RFC: update C++ style to require the "override" keyword
+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 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 >>> >>
Re: RFC: update C++ style to require the "override" keyword
+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 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
Re: RFC: update C++ style to require the "override" keyword
Hi, >> 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'd prefer to hoist the rationale up to our guide, since the google one is > pretty long and I don't expect us to all re-read it regularly :) I can see that and am not strongly opposed to that approach. At the same time that way we’d then require contributors to parse _both_ our and Google’s C++ style guide to understand what style we prefer (plus figuring out the delta). Since in this case we should be able to perform automated checks, it should be possible to educate contributors about this requirement even without explicitly calling it out ourself, so we could avoid putting more reading between them and their contribution. I’ll put up a RR so we mention both `support/mesos-style.py` and `support/mesos-tidy.sh` in the style guide. > look for a review request in the near future Sure! https://silverinahaystack.files.wordpress.com/2016/10/duck-and-cover-2.png Cheers, Benjamin
Re: RFC: update C++ style to require the "override" keyword
> On Jul 8, 2018, at 10:55 PM, Benjamin Bannier > 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'd prefer to hoist the rationale up to our guide, since the google one is pretty long and I don't expect us to all re-read it regularly :) I'll definitely make the tooling changes you suggest (look for a review request in the near future) J
Re: RFC: update C++ style to require the "override" keyword
+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 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
Re: RFC: update C++ style to require the "override" keyword
+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
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
RFC: update C++ style to require the "override" keyword
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
