> On Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 147 > > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line147> > > > > could you use composition instead of inheritance here? It might avoid > > some of the awkward static_cast<...>(*this) constructions. It also means > > that users of the class won't be tempted to try to use the underlying > > interval_set directly.
I agree that using composition is, in general, better than inheritance when extending a core data structure because it avoids some subtle issues (e.g. base class does not have a virtual destructor, etc.). However, using composition makes the code really verbose as you need to provide a proxy for every single method you wanna use. Therefore, in our code base, we use inheritance in many places (e.g. hashmap, hashset, etc.) if we are not adding any new fields. I guess our philology is: more code -> more bugs. The users of this API should not try to use boost directly, which is pretty easy to enforce and check. > On Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 84 > > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line84> > > > > could this be replaced by a method that gets both bounds in a pair? > > > > ie: std::pair<Bound<T>, Bound<T>> bounds() const; We are not fond of std::pair because: To get the left bound, you need to do: pair.first This makes the code a little hard to read. > On Feb. 18, 2014, 5:46 p.m., Dominic Hamon wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp, line 53 > > <https://reviews.apache.org/r/18093/diff/3/?file=487339#file487339line53> > > > > This is clever, but falls in the 'just because you can, doesn't mean > > you should' category. You can get the same result by replacing the factory > > constructors above with: > > > > openOpen, openClosed, closedOpen, closedClosed. > > > > This is more explicit, less verbose, and less surprising. I am OK with both. What do other think here? Please weigh in:) - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18093/#review34713 ----------------------------------------------------------- On Feb. 17, 2014, 1:46 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18093/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2014, 1:46 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 > > 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 > >
