Adding extra argument DCHECKs to AddressList methods.

Adding extra argument DCHECKs to AddressList methods.

BUG=40796
TEST=net_unittests.exe --gtest_filter="*AddressList*"

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

[eroman, 2010-04-15 17:33:58]

LGTM

http://codereview.chromium.org/1560028/diff/2001/3002
File net/base/address_list.h (right):

http://codereview.chromium.org/1560028/diff/2001/3002#newcode66
net/base/address_list.h:66: const struct addrinfo* head() const { return data_ ?
data_->head : NULL; }
Not sure if this buys us much (consumers using an invalid AddressList will
probably just crash later when using the NULL addrinfo. Might actually be better
to just have them crash right away here).

[cbentzel, 2010-04-15 20:50:55]

http://codereview.chromium.org/1560028/diff/2001/3002
File net/base/address_list.h (right):

http://codereview.chromium.org/1560028/diff/2001/3002#newcode66
net/base/address_list.h:66: const struct addrinfo* head() const { return data_ ?
data_->head : NULL; }
On 2010/04/15 17:33:58, eroman wrote:
> Not sure if this buys us much (consumers using an invalid AddressList will
> probably just crash later when using the NULL addrinfo. Might actually be
better
> to just have them crash right away here).

They could possibly check if NULL head and skip using it (I had a case like
this, but moved away from that model).

I'll just limit this CL to introducing the DCHECKs and skip making any actual
behavior changes.

[wtc, 2010-04-16 00:54:25]

LGTM.

http://codereview.chromium.org/1560028/diff/2001/3001
File net/base/address_list.cc (right):

http://codereview.chromium.org/1560028/diff/2001/3001#newcode125
net/base/address_list.cc:125: DCHECK(!head->ai_canonname);
I think only this DCHECK and the DCHECK you added to the
AddressList::Data constructor are useful.

http://codereview.chromium.org/1560028/diff/2001/3002
File net/base/address_list.h (right):

http://codereview.chromium.org/1560028/diff/2001/3002#newcode66
net/base/address_list.h:66: const struct addrinfo* head() const { return data_ ?
data_->head : NULL; }
I agree with eroman that this null check with data_ isn't
useful.  If data_ is NULL, it is a bug in our code.  I found
many callers of AddressList::head() that do not check if
AddressList::head() returns NULL.

[cbentzel, 2010-04-16 14:54:25]

http://codereview.chromium.org/1560028/diff/2001/3001
File net/base/address_list.cc (right):

http://codereview.chromium.org/1560028/diff/2001/3001#newcode125
net/base/address_list.cc:125: DCHECK(!head->ai_canonname);
On 2010/04/16 00:54:25, wtc wrote:
> I think only this DCHECK and the DCHECK you added to the
> AddressList::Data constructor are useful.

I agree that those are the two most useful ones. In the past I've used
DCHECK's/assert style approaches for arguments to act as additional
documentation for what the preconditions are as well as to give clearer error
messages than just segfaults, unless they make the debug build unbearably slow.