> On Feb. 22, 2017, 6: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`? > > Benjamin Bannier wrote: > 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).
I see you already found the new review I posted, but I'll leave this here for posterity :) Would prefer to increase usage of `override` rather than decrease. I posted a review here to make it consistent across the `LibeventSSLSocketImpl` class; let me know what you think! https://reviews.apache.org/r/57160/ - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56945/#review166389 ----------------------------------------------------------- On Feb. 22, 2017, 6: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, 6: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 > >
