Add constructor for creating an AddressList from a struct sockaddr. This is...

Add constructor for creating an AddressList from a struct sockaddr.  This is
preliminary work for using UDP, where we're going to use recvfrom(), which
receives a sockaddr from the network call, and we want to turn that into an
AddressList which is more usable for the app.

BUG=none
TEST=AddressFromAddressInfo

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76489

[eroman, 2011-03-01 02:52:33]

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc#new...
net/base/address_list.cc:262: return NULL;
under what circumstance is this valid?

Seems like we may just be deferring the crash to some other callsite which is
now getting a NULL.

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
address_length,
nit: I have a personal hatred towards constructor overloading.

Could you make this a named static method, like say:

static AddressList CreateFromSockaddr(...);


(I would recommend the same for the constructor call above so it is less
special.... CreateAddressListFromNumber(...), I don't expect you to do that in
this CL tho.)

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list_unitte...
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list_unitte...
net/base/address_list_unittest.cc:236: ASSERT_EQ(0, rv);
I wander why we aren't using net::OK throughout this file. (I realize you are
being consistent with the other test cases, so that is fine).

[eroman, 2011-03-01 02:53:09]

(didnt publish this before it seems)

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
address_length,
Please make it a |const struct sockaddr*| to emphasize it does not take
ownership.

Also please mention in the comment that it

[Mike Belshe, 2011-03-01 19:01:33]

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc#new...
net/base/address_list.cc:262: return NULL;
On 2011/03/01 02:52:33, eroman wrote:
> under what circumstance is this valid?

AddressList foo;
if (foo->head())
   blah blah

This would crash.

I find it inconsistent to a) require that we not overload constructors (which
forces things like uninitialized AddressList() objects) and then b) to not
provide graceful accessors around the object (like in this case).  Now we've got
the weird state of both not being able to construct them and not being able to
tell if you've initialized yourself.


> 
> Seems like we may just be deferring the crash to some other callsite which is
> now getting a NULL.

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
address_length,
On 2011/03/01 02:52:33, eroman wrote:
> nit: I have a personal hatred towards constructor overloading.

I don't understand why - especially for a case like this.

I could see that objection, especially for cases where you end up with redundant
code in the constructor.  But in this case, that isn't true.  Further, I now
lose the ability to create these on the stack, and have to use the heap.  Yuck.

I'll make the change, but I find it arbitrary.

> 
> Could you make this a named static method, like say:
> 
> static AddressList CreateFromSockaddr(...);
> 
> 
> (I would recommend the same for the constructor call above so it is less
> special.... CreateAddressListFromNumber(...), I don't expect you to do that in
> this CL tho.)

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
address_length,
On 2011/03/01 02:53:09, eroman wrote:
> Please make it a |const struct sockaddr*| to emphasize it does not take
> ownership.
> 
> Also please mention in the comment that it 

Done.

[eroman, 2011-03-01 22:11:53]

LGTM (did you upload the latest patchset, I don't see the changes you
described?)

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc
File net/base/address_list.cc (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc#new...
net/base/address_list.cc:262: return NULL;
On 2011/03/01 19:01:33, Mike Belshe wrote:
> On 2011/03/01 02:52:33, eroman wrote:
> > under what circumstance is this valid?
> 
> AddressList foo;
> if (foo->head())
>    blah blah
> 
> This would crash.
> 
> I find it inconsistent to a) require that we not overload constructors (which
> forces things like uninitialized AddressList() objects) and then b) to not
> provide graceful accessors around the object (like in this case).  Now we've
got
> the weird state of both not being able to construct them and not being able to
> tell if you've initialized yourself.
> 
> 
> > 
> > Seems like we may just be deferring the crash to some other callsite which
is
> > now getting a NULL.
> 

I disagree.

I think it is wrong for code to be inspecting whether the object is "valid" or
not. It seems like a logic error if consumers are being handed invalid
AddressLists. They should not have to be checking for this (hence crashing IMO
is the appropriate choice).

In fact I would rather we crash when invoking foo->head(), then to be passing
NULL deep into some system library which may not be handling that, and only
crashing later.

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h
File net/base/address_list.h (right):

http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
address_length,
On 2011/03/01 19:01:33, Mike Belshe wrote:
> On 2011/03/01 02:52:33, eroman wrote:
> > nit: I have a personal hatred towards constructor overloading.
> 
> I don't understand why - especially for a case like this.
> 
> I could see that objection, especially for cases where you end up with
redundant
> code in the constructor.  But in this case, that isn't true.  Further, I now
> lose the ability to create these on the stack, and have to use the heap. 
Yuck.
> 
> I'll make the change, but I find it arbitrary.
> 
> > 
> > Could you make this a named static method, like say:
> > 
> > static AddressList CreateFromSockaddr(...);
> > 
> > 
> > (I would recommend the same for the constructor call above so it is less
> > special.... CreateAddressListFromNumber(...), I don't expect you to do that
in
> > this CL tho.)
> 

The problem I have with constructor overloading, is can hurt readability of
code. When you have just 1 or 2 constructors it is manageable, and I would be OK
with leaving this as a constructor. However once more variants start to get
added, it gets icky.

At the extreme, take the MockRead class, which zillions of constructors. I find
it hurts the readability of that code, since constructors are essentially
unnamed functions (i can never tell what the heck those ctors are doing just by
reading the callsite). Whereas static factory methods solve this by giving named
actions to the creation.

As far as the performance impact, I think that is a non-issue. Ultimately you
are still heap-allocating the "Data" member either way, which will dominate the
cost. (I claim the extra cost of having made a stack copy is negligeable). In
fact in practice when we compile with visual studio's WPO, it is able to
recognize this pattern and will avoid the function-local copy.

Anyhow, I am voicing my preference here, I don't want to fight over these nits
:) Whatever you choose to do is fine for this CL.

[Mike Belshe, 2011-03-01 22:25:54]

On 2011/03/01 22:11:53, eroman wrote:
> LGTM (did you upload the latest patchset, I don't see the changes you
> described?)

oops- uploaded now.


> 
> http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc
> File net/base/address_list.cc (right):
> 
>
http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.cc#new...
> net/base/address_list.cc:262: return NULL;
> On 2011/03/01 19:01:33, Mike Belshe wrote:
> > On 2011/03/01 02:52:33, eroman wrote:
> > > under what circumstance is this valid?
> > 
> > AddressList foo;
> > if (foo->head())
> >    blah blah
> > 
> > This would crash.
> > 
> > I find it inconsistent to a) require that we not overload constructors
(which
> > forces things like uninitialized AddressList() objects) and then b) to not
> > provide graceful accessors around the object (like in this case).  Now we've
> got
> > the weird state of both not being able to construct them and not being able
to
> > tell if you've initialized yourself.
> > 
> > 
> > > 
> > > Seems like we may just be deferring the crash to some other callsite which
> is
> > > now getting a NULL.
> > 
> 
> I disagree.
> 
> I think it is wrong for code to be inspecting whether the object is "valid" or
> not. It seems like a logic error if consumers are being handed invalid
> AddressLists. They should not have to be checking for this (hence crashing IMO
> is the appropriate choice).
> 
> In fact I would rather we crash when invoking foo->head(), then to be passing
> NULL deep into some system library which may not be handling that, and only
> crashing later.


Well, this means that you can't have a AddressList that you fill in later,
without using external state to track whether it is valid.  NULL pointers are a
common way to check for initialization.  You could have another method
"is_initialized() { data_ != NULL; }", I suppose, but like more code for
something trivial.


> 
> http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h
> File net/base/address_list.h (right):
> 
>
http://codereview.chromium.org/6598040/diff/4005/net/base/address_list.h#newc...
> net/base/address_list.h:33: AddressList(struct sockaddr* address, socklen_t
> address_length,
> On 2011/03/01 19:01:33, Mike Belshe wrote:
> > On 2011/03/01 02:52:33, eroman wrote:
> > > nit: I have a personal hatred towards constructor overloading.
> > 
> > I don't understand why - especially for a case like this.
> > 
> > I could see that objection, especially for cases where you end up with
> redundant
> > code in the constructor.  But in this case, that isn't true.  Further, I now
> > lose the ability to create these on the stack, and have to use the heap. 
> Yuck.
> > 
> > I'll make the change, but I find it arbitrary.
> > 
> > > 
> > > Could you make this a named static method, like say:
> > > 
> > > static AddressList CreateFromSockaddr(...);
> > > 
> > > 
> > > (I would recommend the same for the constructor call above so it is less
> > > special.... CreateAddressListFromNumber(...), I don't expect you to do
that
> in
> > > this CL tho.)
> > 
> 
> The problem I have with constructor overloading, is can hurt readability of
> code. When you have just 1 or 2 constructors it is manageable, and I would be
OK
> with leaving this as a constructor. However once more variants start to get
> added, it gets icky.

I don't see any difference.  If you have 3 ways to construct an object, whether
its a static method or a constructor, you generally have the same complexity. 
Constructors have two problems:   they can't return an error code, and they
can't reference another constructor's code (like Java can).  So as a result, you
can get a lot of duplicated code.  But, if that isn't happening, then its
equivalent, and the constructor has some benefits - like in-place
initialization.

> At the extreme, take the MockRead class, which zillions of constructors. I
find
> it hurts the readability of that code, since constructors are essentially
> unnamed functions (i can never tell what the heck those ctors are doing just
by
> reading the callsite). Whereas static factory methods solve this by giving
named
> actions to the creation.

That's true.

> As far as the performance impact, I think that is a non-issue. Ultimately you
> are still heap-allocating the "Data" member either way, which will dominate
the
> cost. (I claim the extra cost of having made a stack copy is negligeable). In
> fact in practice when we compile with visual studio's WPO, it is able to
> recognize this pattern and will avoid the function-local copy.

2 heap allocates vs 1 heap allocate!  I think part of why we both hate this
AddressList structure is that it is a complete mess - constant copying across
multiple pointers and a linked-list chain.

Surely you aren't arguing that the heap is free!
  
> 
> Anyhow, I am voicing my preference here, I don't want to fight over these nits
> :) Whatever you choose to do is fine for this CL.

Already changed to your recommendation.

[eroman, 2011-03-01 22:29:53]

At the risk of beating this thread to death...

> 2 heap allocates vs 1 heap allocate! 

Where are you getting 2 heap allocates from? The |data_| member is reference
counted. You only allocate once in both cases. The copy constructor (assuming it
isn't optimized out) will just increase the refcount...

[Mike Belshe, 2011-03-01 23:16:43]

On 2011/03/01 22:29:53, eroman wrote:
> At the risk of beating this thread to death...
> 
> > 2 heap allocates vs 1 heap allocate! 
> 
> Where are you getting 2 heap allocates from? The |data_| member is reference
> counted. You only allocate once in both cases. The copy constructor (assuming
it
> isn't optimized out) will just increase the refcount...

The new CreateAddressListFromSockaddr() returns a heap allocated object.

Old code:  AddressList foo(<complicated args here>)
New code:  AddressList* foo =
AddressList::CreateAddressListFromSockaddr(<complicated args here>)

So the new code has to do a heap allocation.  I know, you'll say I could have
done
  New code':   AddressList foo;
               AddressList::CreateAddressListFromSockAddr(&foo);

But I don't like that either.

[eroman, 2011-03-04 01:16:40]

Ah, I missed that in my review (I didn't realize you were returning an
AddressList*) from that function.

Why not just return an |AddressList| ?

If you did that, you wouldn't have the extra heap allocation, and the comments I
said earlier will actually make sense.