> On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote: > > 3rdparty/stout/include/stout/ip.hpp > > Lines 208 (patched) > > <https://reviews.apache.org/r/59688/diff/1/?file=1735933#file1735933line209> > > > > Ideally we move this to net::inet::IP::Network::LOOPBACK(), but perhaps > > add a TODO for now if this is not trivial.
Have added a TODO for now, but will create a separate patch for `net::inet::IP::Network` once we have the `inet::IP` patch done. > On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote: > > 3rdparty/stout/include/stout/ip.hpp > > Lines 291-292 (patched) > > <https://reviews.apache.org/r/59688/diff/1/?file=1735933#file1735933line292> > > > > It would be great to have a comment here explaining why you need to use > > `std::unique_ptr` so others don't have to try and read your mind! ;-) I > > don't even always remember the C++ rules here. > > > > Another thought is to use a `std::shared_ptr` here instead. The > > advantage being that maybe then you wouldn't need the extra copy > > constructor? This class is constant, i.e., you never manipulate `address_` > > or `netmask_`, but actually making this `std::shared_ptr<const IP>` might > > be just as tricky. Either way, comment on why this has to be a pointer! > > Avinash sridharan wrote: > Valid point. Will add the comment. > > About `std::unique_ptr` vs `std::shared_ptr`, the argument is valid (not > requiring the copy constructor) and I was thinking about it as well. The > problem I had was that semantically it seemed incorrect (in my opnion). Each > `IP::Network` is an individual object that does not share state with another > copy so thought `std::unique_ptr` better exemplifies this semantic. But I > also agree that this is an implementation detail and the object itself is a > ready only (we do not allow manipulation of the `address_` and `netmask_` > properties). Will defer to your judgement on the use of either. Added the comment explaining why need smart pointers. Please re-open this if you still want to use `shared_ptr` instead of `unique_ptr`. - Avinash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59688/#review176713 ----------------------------------------------------------- On June 2, 2017, 2:23 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59688/ > ----------------------------------------------------------- > > (Updated June 2, 2017, 2:23 a.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Bugs: MESOS-7488 > https://issues.apache.org/jira/browse/MESOS-7488 > > > Repository: mesos > > > Description > ------- > > Moved `net::IPNetwork` to `net::IP:Network`. > > > Diffs > ----- > > 3rdparty/stout/include/stout/ip.hpp > ad2bd922158eecd12b51b272d2a003b8ec8d3550 > 3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e > > > Diff: https://reviews.apache.org/r/59688/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Avinash sridharan > >
