You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/02/28 19:55:33 UTC
Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Joseph Wu.
Repository: mesos
Description
-------
Inconsistent use of the `override` keyword in
`LibeventSSLSocketImpl` was causing warnings during
clang builds. This patch makes use of the keyword
across all relevant declarations in the class.
Diffs
-----
3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
Diff: https://reviews.apache.org/r/57160/diff/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 28, 2017, 10:31 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, lines 47-48
> > <https://reviews.apache.org/r/57160/diff/2/?file=1652008#file1652008line47>
> >
> > With the removal of `virtual`, this fits on one line again :)
doh! thx. I was about to update, but I see you've already submitted :)
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/#review167164
-----------------------------------------------------------
On Feb. 28, 2017, 8:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 8:51 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
>
> Diff: https://reviews.apache.org/r/57160/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/#review167164
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 47 - 48)
<https://reviews.apache.org/r/57160/#comment239335>
With the removal of `virtual`, this fits on one line again :)
3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 51 - 54)
<https://reviews.apache.org/r/57160/#comment239336>
Same whitespace with this line.
- Joseph Wu
On Feb. 28, 2017, 12:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:51 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
>
> Diff: https://reviews.apache.org/r/57160/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 8:51 p.m.)
Review request for mesos, Benjamin Bannier and Joseph Wu.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
Inconsistent use of the `override` keyword in
`LibeventSSLSocketImpl` was causing warnings during
clang builds. This patch makes use of the keyword
across all relevant declarations in the class.
Diffs (updated)
-----
3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
Diff: https://reviews.apache.org/r/57160/diff/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Feb. 28, 2017, 12:27 p.m., Benjamin Bannier wrote:
> > This lgtm. I believe that even applying `override` incompletely can prevent bad mistakes, but am unsure if we are willing to tolerate inconsistencies in the Mesos code base at all. What's your stance on this Joseph?
Since the plan is to eventually have `override` on all overriden functions, I find it better to get there incrementally than not-at-all.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/#review167139
-----------------------------------------------------------
On Feb. 28, 2017, 12:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:51 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
>
> Diff: https://reviews.apache.org/r/57160/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 57160: Added 'override' to function declarations in
'LibeventSSLSocketImpl'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57160/#review167139
-----------------------------------------------------------
Fix it, then Ship it!
This lgtm. I believe that even applying `override` incompletely can prevent bad mistakes, but am unsure if we are willing to tolerate inconsistencies in the Mesos code base at all. What's your stance on this Joseph?
3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 40)
<https://reviews.apache.org/r/57160/#comment239292>
We should apply `override` here and drop `virtual`.
3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 43 - 51)
<https://reviews.apache.org/r/57160/#comment239290>
Since you are about to set a precedent, please drop `virtual` everywhere here. A function which is `override` already necessarily implements a `virtual` function.
3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 63)
<https://reviews.apache.org/r/57160/#comment239291>
Drop `virtual` here as well.
- Benjamin Bannier
On Feb. 28, 2017, 8:55 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 8:55 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp e589a04d14378f265a8fca871c9f5b0c577f5713
>
> Diff: https://reviews.apache.org/r/57160/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>