mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 18093: Added stout/interval.hpp to model boost interval set.
Date Fri, 14 Feb 2014 18:16:06 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message