> On Feb. 5, 2015, 5:53 p.m., Alexander Rojas wrote: > > include/mesos/resources.hpp, lines 301-309 > > <https://reviews.apache.org/r/30654/diff/1/?file=850342#file850342line301> > > > > I'm not sure operator overload is the way to go here. I think it will > > make the code confusing. Example: > > > > ```c++ > > auto persistentVolume = Resources::Filter::persistentVolume(); > > auto reserved = Resources::Filter::reserved(); > > // Which is the return type? > > auto result = reserved && persistenVolume; > > ``` > > > > I would prefer a concatenation method akin two: > > > > ```c++ > > auto persistentVolume = Resources::Filter::persistentVolume(); > > auto reserved = Resources::Filter::reserved(); > > auto result = reserved.and(persistenVolume); > > ``` > > Michael Park wrote: > Thanks for the feedback Alexander! > > My main thing here is composability so I'm not tied to the operator > overload. But the example on top, > > ``` > auto persistentVolume = Resources::Filter::persistentVolume(); > auto reserved = Resources::Filter::reserved(); > // Which is the return type? > auto result = reserved && persistenVolume; > ``` > > All of the return types are `Filter`s. `Filter`s aren't templatized or > anything. > Are you saying that it's weird the return of `reserved && > persistentVolume` isn't a `bool`? I could see a case for that. > > I'd be fine with going with a function instead, but I would prefer > `Filter and(const Filter &lhs, const Filter &rhs)` over `Filter > Filter::and(const Filter &that)` for symmetry.
Yes, that is what I meant. It just doesn't look right to use boolean operators and that the return type is not a boolean. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30654/#review71233 ----------------------------------------------------------- On Feb. 5, 2015, 8:02 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30654/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2015, 8:02 a.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Update the generic filter abstraction for Resources. > > > Diffs > ----- > > include/mesos/resources.hpp 3b57568c10233a0c692787de6464f21af5eaadf4 > src/common/resources.cpp 308b8860fec3db5bfab3925423afda037bfbc4e7 > src/master/hierarchical_allocator_process.hpp > 6b4489276d9f72f3bd99066bf7e48dba5ebe537e > src/tests/hierarchical_allocator_tests.cpp > f44d9e98d6d9db9621f5361cddb6134f90277180 > src/tests/resources_tests.cpp 4744e872b082553046ecc0e344403754ee685842 > > Diff: https://reviews.apache.org/r/30654/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Michael Park > >
