mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 18093: Added stout/interval.hpp to model boost interval set.
Date Tue, 04 Mar 2014 01:36:51 GMT

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


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