Style refactoring to make MockRead initializers more readable.

Style refactoring to make MockRead initializers more readable.


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

[darin (slow to review), 2008-09-29 06:14:34]

OK, LGTM

[wtc, 2008-09-29 20:16:33]

LGTM.

I actually find it equally hard to remember the four members
in the initializer list and the four flavors of MockRead
constructors.

http://codereview.chromium.org/5026/diff/1/2
File net/http/http_network_transaction_unittest.cc (left):

http://codereview.chromium.org/5026/diff/1/2#oldcode32
Line 32: MockRead* reads;  // Terminated by a MockRead element with data ==
NULL.
Do we still need to terminate the 'reads' array with a
MockRead element with data == NULL?

Or is this comment actually wrong?

http://codereview.chromium.org/5026/diff/1/2
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/5026/diff/1/2#newcode23
Line 23: MockConnect() : async(true), result(net::OK) { }
Nit: list the constructor before the members, as you do
in MockRead and MockSocket.

http://codereview.chromium.org/5026/diff/1/2#newcode41
Line 41: data(data), data_len(data_len), result(0) { }
Ideally, these should be accompanied with the C++ machinery
to ensure that MockRead can only be constructed in these
four ways.  In particular, we should disallow initializing
with an initializer list like { false, net::OK, NULL, 0 }.

Do you know how to do that?  Just curious.

[eroman, 2008-09-29 20:29:17]

> I actually find it equally hard to remember the four
> members in the initializer list and the four flavors
> of MockRead constructors.

darn. note that only 2 of the forms are actually used:

MockRead(const char* data) // async with data
MockRead(bool async, int result) // no data

(the other two are for completeness).

http://codereview.chromium.org/5026/diff/1/2
File net/http/http_network_transaction_unittest.cc (left):

http://codereview.chromium.org/5026/diff/1/2#oldcode32
Line 32: MockRead* reads;  // Terminated by a MockRead element with data ==
NULL.
On 2008/09/29 20:16:33, wtc wrote:
> Do we still need to terminate the 'reads' array with a
> MockRead element with data == NULL?
> 
> Or is this comment actually wrong?

as far as I can tell, that comment was wrong -- none of the tests do this. (they
do however terminate mock_sockets with a null entry).

http://codereview.chromium.org/5026/diff/1/2
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/5026/diff/1/2#newcode23
Line 23: MockConnect() : async(true), result(net::OK) { }
On 2008/09/29 20:16:33, wtc wrote:
> Nit: list the constructor before the members, as you do
> in MockRead and MockSocket.

oops. will fix.
(I already submitted this as I rely on it in another CL. I am fixing it in that
CL).

http://codereview.chromium.org/5026/diff/1/2#newcode41
Line 41: data(data), data_len(data_len), result(0) { }
On 2008/09/29 20:16:33, wtc wrote:
> Ideally, these should be accompanied with the C++ machinery
> to ensure that MockRead can only be constructed in these
> four ways.  In particular, we should disallow initializing
> with an initializer list like { false, net::OK, NULL, 0 }.
> 
> Do you know how to do that?  Just curious.

That happens automatically -- once you have a user-defined constructor, the type
cannot be initialized with an initializer list.