> On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > include/mesos/resources.hpp, lines 102-103 > > <https://reviews.apache.org/r/28089/diff/2/?file=766043#file766043line102> > > > > Can you put implicit on the same line? Like future.hpp, for example.
Done. > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 66 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line66> > > > > Let's stop saying superset :) > > > > How about: > > > > // Returns iff 'right' is contained in 'left'. > > > > However, shouldn't this comment be in the header? It's static private functions:) > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 54 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line54> > > > > Let's avoid saying superset now, let's just say contains :) > > > > How about: > > > > ``` > > // NOTE: Set substraction is always well defined, it does not > > // require 'right' to be contained within 'left'. > > ``` Done. > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, lines 89-91 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line89> > > > > How about: > > > > ``` > > if (left.name() != right.name() || > > left.type() != right.type() || > > left.role() != right.role()) { > > return false; > > } > > ``` > > > > We've been avoiding these one-liners I think. Done. > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 113 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line113> > > > > Can this one be removed? Is it used now that it is hidden in the .cpp > > file? Nope, all tests still rely on this operator. Will be removed in https://reviews.apache.org/r/28094/ > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 131 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line131> > > > > Hm.. I assume you removed the call to `matches()` intentionally because > > of some non-local reasoning? Why is it safe to assume that the name, type, > > and role match? > > > > If you really don't want to check matching, we should at least have a > > comment! Ditto for -=, hard to tell locally why it doesn't check the name > > and role. My bad. But given https://reviews.apache.org/r/28092/, we will remove the comments in that patch. I'll make sure I commit the patches in a whole. > On Nov. 17, 2014, 10:12 p.m., Ben Mahler wrote: > > src/common/resources.cpp, line 133 > > <https://reviews.apache.org/r/28089/diff/2/?file=766044#file766044line133> > > > > Can you add some TODO's to leverage the += and -= operators within > > these? You'll have to fix the bugs in the Values operators first, but > > dropping a TODO for context in this patch would be great :) Done. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28089/#review61802 ----------------------------------------------------------- On Nov. 17, 2014, 8:34 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28089/ > ----------------------------------------------------------- > > (Updated Nov. 17, 2014, 8:34 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-1974 > https://issues.apache.org/jira/browse/MESOS-1974 > > > Repository: mesos-git > > > Description > ------- > > Hide operators for Resource object. Split "matches" to handle future first > class infos. > > > Diffs > ----- > > include/mesos/resources.hpp 0e37170262d3470570a3436b7835bb1d4a121056 > src/common/resources.cpp e9a0c85dc3748aa635843d98cd5993d5648ec5c2 > src/tests/resources_tests.cpp 3e50889ff2433930bd137a9d836d0a8bfc2d0388 > > Diff: https://reviews.apache.org/r/28089/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
