mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 59688: Moved `net::IPNetwork` to `net::IP:Network`.
Date Fri, 02 Jun 2017 02:25:33 GMT


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


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