> On Nov. 15, 2014, 11 p.m., Michael Park wrote: > > src/common/resources.cpp, lines 69-71 > > <https://reviews.apache.org/r/28089/diff/1/?file=764759#file764759line69> > > > > How about using `combinable` here? > > `if (!combinable(left, right)) { return false; }`
With DiskInfo that will be added in the near future, this function will be changed so that it's not quite easy to re-use combinable here. So I would rather not to change it here. > On Nov. 15, 2014, 11 p.m., Michael Park wrote: > > src/common/resources.cpp, lines 89-91 > > <https://reviews.apache.org/r/28089/diff/1/?file=764759#file764759line89> > > > > Ditto here, use `combinable`? Ditto my comments above. > On Nov. 15, 2014, 11 p.m., Michael Park wrote: > > src/common/resources.cpp, line 133 > > <https://reviews.apache.org/r/28089/diff/1/?file=764759#file764759line133> > > > > This variable isn't necessary right? Will follow up with a patch to fix this and a BUG in src/common/value.cpp > On Nov. 15, 2014, 11 p.m., Michael Park wrote: > > src/common/resources.cpp, line 137 > > <https://reviews.apache.org/r/28089/diff/1/?file=764759#file764759line137> > > > > Hm, just curious. Do we need the `Clear` here even if we're now doing > > `CopyFrom`? Ditto. Will clean it up. > On Nov. 15, 2014, 11 p.m., Michael Park wrote: > > include/mesos/resources.hpp, lines 102-106 > > <https://reviews.apache.org/r/28089/diff/1/?file=764758#file764758line102> > > > > I would suggest keeping the behavior symmetric to `std::vector` here. > > I'm guessing this was added due to the annoyance around initialization. The > > same issue existed for `std::vector` in `C++98` but it's been fixed with > > `std::initializer_list` constructors in `C++11`. We can implement one for > > `Resources` as well. > > > > ```cpp > > // C++98 > > std::vector<int> v; > > v.push_back(1); > > v.push_back(2); > > > > // C++11 > > std::vector<int> v = {1, 2}; > > ``` > > > > For `Resources`, it would look like: > > > > ```cpp > > Resources(std::initializer_list<Resource> resources) { > > foreach (const Resource& resource, resources) { > > *this += resource; > > } > > } > > ``` > > > > Then we can do the following transformation: > > > > ```cpp > > // Before > > Resource cpus1 = Resources::parse("cpus", "1", "role1").get(); > > Resources r1; > > r1 += cpus1; > > > > // After > > Resources r1 = { Resources::parse("cpus", "1", "role1").get() }; > > ``` I'll add a TODO. Let's clean it up once we allow initialization list. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28089/#review61667 ----------------------------------------------------------- On Nov. 15, 2014, 6:56 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28089/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2014, 6:56 a.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 0e37170 > src/common/resources.cpp e9a0c85 > src/tests/resources_tests.cpp 3e50889 > > Diff: https://reviews.apache.org/r/28089/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
