I notice we sweep update the code which the style is incorrect. For example, replace "A<<B> >" with "A<<B>>", replace ".get()" to "->", remove incorrect space, adjust line length. Does this mean we need to split them out as an additional patch?
On Sun, May 8, 2016 at 8:38 AM, Benjamin Mahler <[email protected]> wrote: > Hm.. any reason that unrelated headers were touched and the using statement > was removed in this patch? > > My concern with mixing unrelated changes within a single patch is that > patches become less precise. If one reads the patch there is additional > overhead in understanding what is related to the goal of the change and > what is not. I know it's a small example here but I see value in being > disciplined about this regardless of patch size. > > The other concern is that the reviewer of this patch has to review these > two additional changes: > 1. How does the header audit look? Anything else need added or removed? > 2. How does the 'using' audit look? Anything else need added or removed? > > (1) and (2) could be done together in a single patch. As in turns out, the > header audit looks like it has a few issues, but I'm guessing the reviewer > glossed over it because the point of this change was CHECK_READY :) > -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used > -<glog/logging.h> is not present but LOG is used > -<stout/nothing.hpp> is absent but Nothing is used > -<process/process.hpp> is absent but process::Process / process::wait / > process::terminate are used. > > Then for the 'using' audit, we now avoid pulling in all of the process:: > namespace in favor of finer-grained using statements. > > On Mon, May 2, 2016 at 4:08 AM, <[email protected]> wrote: > > > Repository: mesos > > Updated Branches: > > refs/heads/master 78f6101cc -> 4f9040db6 > > > > > > Replaced CHECK with CHECK_READY. > > > > Also removes some unused header includes. > > > > Review: https://reviews.apache.org/r/46827/ > > > > > > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo > > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db > > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db > > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db > > > > Branch: refs/heads/master > > Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310 > > Parents: 78f6101 > > Author: Neil Conway <[email protected]> > > Authored: Mon May 2 13:05:56 2016 +0200 > > Committer: Alexander Rukletsov <[email protected]> > > Committed: Mon May 2 13:05:56 2016 +0200 > > > > ---------------------------------------------------------------------- > > src/zookeeper/contender.cpp | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > ---------------------------------------------------------------------- > > > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp > > ---------------------------------------------------------------------- > > diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp > > index 4b1cc65..5eb4b58 100644 > > --- a/src/zookeeper/contender.cpp > > +++ b/src/zookeeper/contender.cpp > > @@ -14,26 +14,23 @@ > > // See the License for the specific language governing permissions and > > // limitations under the License > > > > -#include <set> > > #include <string> > > > > #include <mesos/zookeeper/contender.hpp> > > #include <mesos/zookeeper/detector.hpp> > > #include <mesos/zookeeper/group.hpp> > > > > +#include <process/check.hpp> > > #include <process/defer.hpp> > > #include <process/dispatch.hpp> > > #include <process/future.hpp> > > #include <process/id.hpp> > > > > -#include <stout/check.hpp> > > #include <stout/lambda.hpp> > > #include <stout/option.hpp> > > -#include <stout/some.hpp> > > > > using namespace process; > > > > -using std::set; > > using std::string; > > > > namespace zookeeper { > > @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel() > > > > void LeaderContenderProcess::cancelled(const Future<bool>& result) > > { > > - CHECK(candidacy.isReady()); > > + CHECK_READY(candidacy); > > LOG(INFO) << "Membership cancelled: " << candidacy.get().id(); > > > > // Can be called as a result of either withdraw() or server side > > > > > -- Best Regards, Haosdent Huang
