----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18093/#review36067 -----------------------------------------------------------
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? 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp <https://reviews.apache.org/r/18093/#comment66930> 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? 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp <https://reviews.apache.org/r/18093/#comment66932> Maybe some inline comments here? right_open_interval // Target. is_discrete<T> static_open // Input bounds. static_right_open // Output bounds. 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp <https://reviews.apache.org/r/18093/#comment66900> Could we kill this and just use std::distance in the tests? 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp <https://reviews.apache.org/r/18093/#comment66944> 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. - Ben Mahler 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 > >
