> 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'. > > Jie Yu wrote: > 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? > > Ben Mahler wrote: > Sounds great, given your comment can you also add interval iteration in > the tests? I'm curious what that looks like. > > // Would be nice to be able to do something like this for iteration. > foreach (const Interval& interval, intervalSet) { > .. > } > > Jie Yu wrote: > You have a good point. I haven't thought about the interval iteration > enough. To do the interval iteration now, you'll have to do the following: > > foreach (const discrete_interval<T>& interval, intervalSet) { > T left = interval.lower(); > T right = interval.upper(); > > if (interval.bounds() == interval_bounds::right_open()) { > > ... > } else if (interval.bounds() == interval_bounds::closed()) { > ... > } > ... > } > > Seems that we need to abstract Interval as well, which might need to > create a customized interval type. > > http://www.boost.org/doc/libs/1_53_0/libs/icl/doc/html/boost_icl/examples/custom_interval.html > > Any suggestion? > > Ben Mahler wrote: > Hm, is it possible to have Interval inherit from discrete_interval, but > provide simpler accessors for upper() and lower()? > > What I would find most intuitive is an inclusive (closed) lower() and > exclusive (open) upper(), since this is how things like begin() and end() > work. With these, the caller does not need to know about the bounds(). > > Jie Yu wrote: > I did a research on how to provide custom interval (not trivial, but > doable). This is the only option that allows us to do the following: > > foreach (const Interval<T>& interval, intervalSet) { ... } > > We can do whatever we want in class Interval (e.g. as you suggested). > However, when creating intervals, we need to do the following (by specifying > the type): > > intervalSet += Interval<int>::open(2).closed(4); // Notice that we need > to specify '<int>' here. > > Or, we can create another class called "IntervalBuilder": > > intervalSet += IntervalBuilder::open(3).closed(4); > > Which one do you guys prefer? >
I think requiring the type qualifier is an acceptable tradeoff for achieving clean iteration. So I would avoid the separate IntervalBuilder for now. - Ben ----------------------------------------------------------- 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 > >
