> On Feb. 22, 2017, 7:58 p.m., Greg Mann wrote: > > Is the motivation here to eliminate the clang warnings? IIUC having the > > `override` here, while inconsistent, is strictly an improvement from the > > previous state without `override`?
Yes, we currently emit warnings in the clang build without being interested in fixing this, potentially causing other more interesting warnings to slip through (libprocess is not built with `-Werror`). An alternative fix would be to use `override` correctly here, i.e., mark all overriding methods with as `override` and to drop the now completely redundant `virtual`. I agree that this is a strict improvement over the previous state, but I feel implementing MESOS-4871 instead would be the proper fix instead of introducing it incompletely (incompletely both on the code base, and even incompletely on a class here). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56945/#review166389 ----------------------------------------------------------- On Feb. 22, 2017, 7:50 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56945/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 7:50 p.m.) > > > Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > This removes inconsistent use of the 'override' keyword where the > 'LibeventSSLSocketImpl' class contained both a method explicitly > marked 'override' and implicitly 'override' functions. This is > diagnosed as an inconsistency by clang-4.0, e.g., > > /PATH/libevent_ssl_socket.hpp|43 col 27| warning: \ > 'connect' overrides a member function but is not marked \ > 'override' [-Winconsistent-missing-override] > virtual Future<Nothing> connect(const Address& address); > ^ > /PATH/socket.hpp|148 col 27| note: \ > overridden virtual function is here > virtual Future<Nothing> connect(const Address& address) = 0; > > A proper fix will be implemented as part of MESOS-4871. > > > Diffs > ----- > > 3rdparty/libprocess/src/libevent_ssl_socket.hpp > e589a04d14378f265a8fca871c9f5b0c577f5713 > > Diff: https://reviews.apache.org/r/56945/diff/ > > > Testing > ------- > > make check (OS X, clang/trunk). > > > Thanks, > > Benjamin Bannier > >
