> On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 194-207 > > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line194> > > > > Interesting that they provided both + and |, are they not the same > > operation? (In set.hpp, we provided | & as the set operators, and + for > > inserting single elements).
They are the same! "operator |= and operator | are behavioral identical to operator += and operator +. This is a redundancy that has been introduced deliberately, because a set union semantics is often attached operators |= and |." > On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 138-139 > > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line138> > > > > non-const for copying, or? Should be "const T left". Will fix that. > On Feb. 14, 2014, 5:54 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, lines 143-146 > > <https://reviews.apache.org/r/18093/diff/2/?file=484865#file484865line143> > > > > I'm wondering about the use-cases here to see if the current iterator > > semantics are the easiest to work with. > > > > Do you see most people using this as a plain 'set' (with a compact > > representation) or mostly as a 'set of intervals', or both? > > > > For the first use case, I can imagine calling this 'IntervalSet', but > > exposing the same 'Set' like interface as, say, hashset. > > > > For the second use case, I can imagine calling this 'Intervals', and > > exposing the current interface. > > > > For example, what should the default iterator behavior be? > > > > // Case 1: > > foreach (const T& t, set) { > > ... > > } > > > > // Case 2: > > foreach (const Interval& i, intervals) { > > ... > > } > > > > Just something to think about. It seems like you will be using this > > mostly as a 'Set'. There is a note in the doc: "So we advice you to choose element iteration over interval containers judiciously. Do not use element iteration by default or habitual. Always try to achieve results using member functions, global functions or operators (preferably inplace versions) or iteration over segments first." I guess the reason is because doing iteration on elements are somewhat dangerous because you might have a very large interval (e.g. imaging that you insert an interval [1,100000000]). Therefore, I think maybe we should stick to the original boost semantics and make element iteration explicit. Also, changing the default behavior of begin()/end() is a bit tricky since you cannot simply extend from boost interval_set anymore :( Regarding the Intervals idea, I liked that! My concern is: we haven't seen use cases currently. Maybe we should delay that until we see some actual use cases which then can help us design a good API. What do you think? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18093/#review34507 ----------------------------------------------------------- On Feb. 14, 2014, 5:38 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18093/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2014, 5:38 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/Makefile.am 51b118c > 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 > > > Thanks, > > Jie Yu > >
