You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2018/07/10 03:36:41 UTC
Review Request 67869: Add use of `override` to the Mesos C++ style
guide.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
Bugs: MESOS-9065
https://issues.apache.org/jira/browse/MESOS-9065
Repository: mesos
Description
-------
Add an explicit guideline to the Mesos C++ style guide that the use of
`override` keyword is required. Update cpplint and clang-tidy tooling
to encourage this.
Diffs
-----
docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
Diff: https://reviews.apache.org/r/67869/diff/1/
Testing
-------
Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
Thanks,
James Peach
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review205940
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67866, 67867, 67868, 67869]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On July 10, 2018, 3:36 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 10, 2018, 3:36 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/1/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review205892
-----------------------------------------------------------
PASS: Mesos patch 67869 was successfully built and tested.
Reviews applied: `['67866', '67867', '67868', '67869']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1892/mesos-review-67869
- Mesos Reviewbot Windows
On July 9, 2018, 8:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 9, 2018, 8:36 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/1/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 16, 2018, 1:31 p.m., Benjamin Bannier wrote:
> > docs/c++-style-guide.md
> > Lines 651-662 (patched)
> > <https://reviews.apache.org/r/67869/diff/2/?file=2058877#file2058877line651>
> >
> > Like I already mentioned on the mailing list, I am still not a fan of adding redundant information which is already in the Google style guide here. That way we'd ask ideal contributors to read the Google style guide, our style guide, and figure out where any the delta is.
IMHO it cannot be reasonably expected that a dev is going to read the entire Google Style Guide. I refer to it plenty often, but if we think this part is _important_, then I think that it is on us to call it out by adding it here.
Sure, the _ideal_ contributor thinks about every possible syntactical rule or formatting rule and tries to find the answer as they write it (I do this, but I'm crazy, and still get it wrong a lot), but realistically I think the most we can expect is that this guide is read, and hopefully it sinks in as "gotchas" that we care most about.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review206124
-----------------------------------------------------------
On July 11, 2018, 10:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 11, 2018, 10:03 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/2/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review206124
-----------------------------------------------------------
Ship it!
docs/c++-style-guide.md
Lines 651-662 (patched)
<https://reviews.apache.org/r/67869/#comment289032>
Like I already mentioned on the mailing list, I am still not a fan of adding redundant information which is already in the Google style guide here. That way we'd ask ideal contributors to read the Google style guide, our style guide, and figure out where any the delta is.
- Benjamin Bannier
On July 12, 2018, 7:03 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 12, 2018, 7:03 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/2/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review206133
-----------------------------------------------------------
Ship it!
As mentioned in my reply to Ben, I am _for_ this change, as I believe that if we care about it, we need to call it out in addition to its presence in the Google Style Guide (that thing is enormous!).
- Andrew Schwartzmeyer
On July 11, 2018, 10:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 11, 2018, 10:03 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/2/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review205997
-----------------------------------------------------------
PASS: Mesos patch 67869 was successfully built and tested.
Reviews applied: `['67866', '67867', '67868', '67869']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1905/mesos-review-67869
- Mesos Reviewbot Windows
On July 12, 2018, 5:03 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 12, 2018, 5:03 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/2/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/#review206040
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67866, 67867, 67868, 67869]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On July 12, 2018, 5:03 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67869/
> -----------------------------------------------------------
>
> (Updated July 12, 2018, 5:03 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
>
>
> Bugs: MESOS-9065
> https://issues.apache.org/jira/browse/MESOS-9065
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add an explicit guideline to the Mesos C++ style guide that the use of
> `override` keyword is required. Update cpplint and clang-tidy tooling
> to encourage this.
>
>
> Diffs
> -----
>
> docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
> support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
> support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
>
>
> Diff: https://reviews.apache.org/r/67869/diff/2/
>
>
> Testing
> -------
>
> Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67869: Add use of `override` to the Mesos C++
style guide.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67869/
-----------------------------------------------------------
(Updated July 12, 2018, 5:03 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Greg Mann, Mesos Reviewbot, Till Toenshoff, and Zhitao Li.
Bugs: MESOS-9065
https://issues.apache.org/jira/browse/MESOS-9065
Repository: mesos
Description
-------
Add an explicit guideline to the Mesos C++ style guide that the use of
`override` keyword is required. Update cpplint and clang-tidy tooling
to encourage this.
Diffs (updated)
-----
docs/c++-style-guide.md 80a1af318984d47bdfa641aa12e28963d3183fea
support/clang-tidy fcffb39089573a708dc3f2052f639a1a3621040d
support/mesos-style.py 27ed553cb1d9e0c3c750b414eafe0144c3442c43
Diff: https://reviews.apache.org/r/67869/diff/2/
Changes: https://reviews.apache.org/r/67869/diff/1-2/
Testing
-------
Manual. The `cpplint` change doesn't appear to enforce the use of `override`.
Thanks,
James Peach