> On March 4, 2014, 1:36 a.m., Ben Mahler wrote: > > I think the only remaining bit that might be tricky is that we expose > > 'Bound' for 'Interval' construction but then we only expose inclusive > > lower() and exclusive upper(). I think this is better than exposing the > > 'Bounds' from interval given the inherent complexity that arises from > > checking all 'Bound' combinations, so LGTM. > > > > I would be curious to know whether you ended up using different 'Bound' > > types in your code that uses IntervalSet. Was it useful?
Please refer to the following review. Seems that we use different Bound types in different cases. Therefore, I think it's useful to expose all combinations. https://reviews.apache.org/r/18368/ src/log/replica.cpp +276 positions += (Bound<uint64_t>::open(end), Bound<uint64_t>::closed(to)); src/log/replica.cpp +280 positions &= (Bound<uint64_t>::closed(from), Bound<uint64_t>::closed(to)); src/log/replica.cpp +679 holes -= (Bound<uint64_t>::open(0), Bound<uint64_t>::open(action.truncate().to())); > On March 4, 2014, 1:36 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 167-172 > > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line167> > > > > Could we kill this and just use std::distance in the tests? In fact, i wanna expose this function and remove the TODO. It makes sense to expose the number of intervals given that we have exposed Interval. What do you think? > On March 4, 2014, 1:36 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 174-187 > > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line174> > > > > In set.hpp, we provided '+' for individual item additions and '|' for > > set union. hashset.hpp also provides only '|' for set union. > > > > I would prefer to only see 1 way to do union, even though boost > > supplies both operators for union, we should restrict the number of ways of > > doing the same thing. > > > > How about we: > > -only use '|' (as Python does: > > http://docs.python.org/2/library/sets.html#set-objects), or > > -only use '+' (inconsistent with (hash)set.hpp), or > > -use | for IntervalSet, + for T, + for Interval (this seems confusing > > and I'm not sure we made the right decision in set.hpp). > > > > Seems like we could avoid potential confusion here where one wonders if > > + and | are different operators. Removed '|' to avoid confusion. > On March 4, 2014, 1:36 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 90-91 > > <https://reviews.apache.org/r/18093/diff/9/?file=507713#file507713line90> > > > > Can you add a comment as to why you are storing the right_open_interval > > as opposed to inheriting from it? Would the latter be cleaner? No convincing reason:) Just don't wanna Interval to be passed to bare boost icl containers. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18093/#review36067 ----------------------------------------------------------- On March 1, 2014, 7:31 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18093/ > ----------------------------------------------------------- > > (Updated March 1, 2014, 7:31 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-993 > https://issues.apache.org/jira/browse/MESOS-993 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/Makefile.am 5d5a760 > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/interval_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18093/diff/ > > > Testing > ------- > > make check > > repeat 1000 times for the IntervalTest > > Repeating all tests (iteration 100) . . . > > Note: Google Test filter = *Interval* > [==========] Running 7 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 7 tests from IntervalTest > [ RUN ] IntervalTest.Interval > [ OK ] IntervalTest.Interval (0 ms) > [ RUN ] IntervalTest.Addition > [ OK ] IntervalTest.Addition (0 ms) > [ RUN ] IntervalTest.Subtraction > [ OK ] IntervalTest.Subtraction (0 ms) > [ RUN ] IntervalTest.Intersection > [ OK ] IntervalTest.Intersection (0 ms) > [ RUN ] IntervalTest.LargeInterval > [ OK ] IntervalTest.LargeInterval (0 ms) > [ RUN ] IntervalTest.ElementIteration > [ OK ] IntervalTest.ElementIteration (0 ms) > [ RUN ] IntervalTest.IntervalIteration > [ OK ] IntervalTest.IntervalIteration (0 ms) > [----------] 7 tests from IntervalTest (0 ms total) > > [----------] Global test environment tear-down > [==========] 7 tests from 1 test case ran. (0 ms total) > [ PASSED ] 7 tests. > > > Thanks, > > Jie Yu > >
