Move net/base/sys_byteorder.h to base/sys_byteorder.h

Move net/base/sys_byteorder.h to base/sys_byteorder.h

Two motivations:
  (1) There are currently clients in src/crypto that need the same logic.
  (2) There is soon to be a client in src/chrome/common that needs the 64-bit version of this logic, which is currently inlined in a src/crypto implementation file.

BUG=103480
TEST=compiles


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

[Ilya Sherman, 2011-12-21 02:13:07]

Brett, are you ok with this hearder's new location, or do you think there's a
better place to put it?

[Ryan Sleevi, 2011-12-21 02:18:37]

http://codereview.chromium.org/8949026/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8949026/diff/1/base/base.gypi#newcode294
base/base.gypi:294: 'base/sys_byteorder.h',
typo: base/sys_byteorder.h -> sys_byteorder.h

http://codereview.chromium.org/8949026/diff/1/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/1/base/sys_byteorder.h#newcode14
base/sys_byteorder.h:14: #define NET_BASE_SYS_BYTEORDER_H_
nit: NET_BASE_SYS_BYTEORDER_H_ -> BASE_SYS_BYTEORDER_H_

[Ilya Sherman, 2011-12-21 02:22:13]

Thanks for catching those -- both fixed.

http://codereview.chromium.org/8949026/diff/1/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/8949026/diff/1/base/base.gypi#newcode294
base/base.gypi:294: 'base/sys_byteorder.h',
On 2011/12/21 02:18:38, Ryan Sleevi wrote:
> typo: base/sys_byteorder.h -> sys_byteorder.h

Done.

http://codereview.chromium.org/8949026/diff/1/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/1/base/sys_byteorder.h#newcode14
base/sys_byteorder.h:14: #define NET_BASE_SYS_BYTEORDER_H_
On 2011/12/21 02:18:38, Ryan Sleevi wrote:
> nit: NET_BASE_SYS_BYTEORDER_H_ -> BASE_SYS_BYTEORDER_H_

Done.

[brettw, 2011-12-21 17:48:13]

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h#newcode27
base/sys_byteorder.h:27: #define bswap_16(x) _byteswap_ushort(x)
Did we consider writing real inline functions rather than #defines for this? I'd
prefer base::ByteSwap() (could be overloaded for 16, 32, and 64-bit types). Or
would this mess something up because the swaps are some kind of intrinsics?

[Ilya Sherman, 2011-12-21 19:43:07]

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h#newcode27
base/sys_byteorder.h:27: #define bswap_16(x) _byteswap_ushort(x)
On 2011/12/21 17:48:14, brettw wrote:
> Did we consider writing real inline functions rather than #defines for this?
I'd
> prefer base::ByteSwap() (could be overloaded for 16, 32, and 64-bit types). Or
> would this mess something up because the swaps are some kind of intrinsics?

+hclam, who originally added these, and probably has better insight into this
question

[Ryan Sleevi, 2011-12-21 19:49:10]

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h#newcode27
base/sys_byteorder.h:27: #define bswap_16(x) _byteswap_ushort(x)
On 2011/12/21 19:43:08, Ilya Sherman wrote:
> On 2011/12/21 17:48:14, brettw wrote:
> > Did we consider writing real inline functions rather than #defines for this?
> I'd
> > prefer base::ByteSwap() (could be overloaded for 16, 32, and 64-bit types).
Or
> > would this mess something up because the swaps are some kind of intrinsics?
> 
> +hclam, who originally added these, and probably has better insight into this
> question

wtc is probably a better candidate, as he reviewed and suggested the use of
these functions.

Definitely for MSVC, they are compiler intrinsics. I haven't checked the object
code to see if putting them in an inline function would interfere with the
intrinsics, but I would suspect it would all optimize out properly at even the
most basic of optimizations.

[wtc, 2011-12-23 00:37:21]

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h#newcode27
base/sys_byteorder.h:27: #define bswap_16(x) _byteswap_ushort(x)
On 2011/12/21 17:48:14, brettw wrote:
> Did we consider writing real inline functions rather than #defines for this?
I'd
> prefer base::ByteSwap() (could be overloaded for 16, 32, and 64-bit types).

No, we didn't consider writing real inline functions.  It
was an oversignt.

[Ilya Sherman, 2011-12-23 02:31:28]

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/29/base/sys_byteorder.h#newcode27
base/sys_byteorder.h:27: #define bswap_16(x) _byteswap_ushort(x)
On 2011/12/23 00:37:22, wtc wrote:
> On 2011/12/21 17:48:14, brettw wrote:
> > Did we consider writing real inline functions rather than #defines for this?
> I'd
> > prefer base::ByteSwap() (could be overloaded for 16, 32, and 64-bit types).
> 
> No, we didn't consider writing real inline functions.  It
> was an oversignt.

Ok, wrote these as real inline functions instead.  There aren't any clients of
BaseSwap() other than the ntoh and hton functions, so I only defined the 64-bit
one.

I also renamed the "ntoh_64" and "hton_64" functions to be "ntohll" and
"htonll", as that is more consistent with the names "ntohs", "htons", "ntohl",
and "htonl" for the 16- and 32-bit versions of these functions.

[brettw, 2011-12-25 00:16:01]

http://codereview.chromium.org/8949026/diff/14003/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/14003/base/sys_byteorder.h#newcode50
base/sys_byteorder.h:50: inline uint64_t ntohll(uint64_t x) {
Can these have documentation? I've never heard of this function. Personally, I
would have called this NetworkEndianToHost or something. If we think this is
more clear because the people using this are more used to ntohll, that's OK, but
we should at least have a description of what it's for for the majority of us
that don't.

[Ilya Sherman, 2011-12-26 08:05:25]

http://codereview.chromium.org/8949026/diff/14003/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/14003/base/sys_byteorder.h#newcode50
base/sys_byteorder.h:50: inline uint64_t ntohll(uint64_t x) {
On 2011/12/25 00:16:01, brettw wrote:
> Can these have documentation? I've never heard of this function. Personally, I
> would have called this NetworkEndianToHost or something. If we think this is
> more clear because the people using this are more used to ntohll, that's OK,
but
> we should at least have a description of what it's for for the majority of us
> that don't.

Added some documentation.  I'd prefer to keep this name for consistency with
ntohs and ntohl.  Or, we could add wrappers for the 16-bit and 32-bit versions
of the function... but I'm not sure that it's worth the code churn.

[brettw, 2011-12-26 18:36:31]

lgtm

[Ilya Sherman, 2011-12-28 00:14:36]

Seeking OWNERS reviews for src/crypto (Ryan?), src/net (Eric?), and src/jingle
(Fred?).

[Ryan Sleevi, 2011-12-28 00:29:25]

crypto LGTM

[eroman, 2011-12-28 00:50:20]

LGTM for net/*

http://codereview.chromium.org/8949026/diff/19001/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/19001/base/sys_byteorder.h#newcode25
base/sys_byteorder.h:25: // Include headers to provide bswap for all platforms.
nit: "bswap" isn't very descriptive

http://codereview.chromium.org/8949026/diff/19001/net/spdy/spdy_frame_builder.h
File net/spdy/spdy_frame_builder.h (right):

http://codereview.chromium.org/8949026/diff/19001/net/spdy/spdy_frame_builder...
net/spdy/spdy_frame_builder.h:11: #include "base/sys_byteorder.h"
nit: order lexicographically.

[Ilya Sherman, 2011-12-28 01:20:00]

http://codereview.chromium.org/8949026/diff/19001/base/sys_byteorder.h
File base/sys_byteorder.h (right):

http://codereview.chromium.org/8949026/diff/19001/base/sys_byteorder.h#newcode25
base/sys_byteorder.h:25: // Include headers to provide bswap for all platforms.
On 2011/12/28 00:50:20, eroman wrote:
> nit: "bswap" isn't very descriptive

Done.

http://codereview.chromium.org/8949026/diff/19001/net/spdy/spdy_frame_builder.h
File net/spdy/spdy_frame_builder.h (right):

http://codereview.chromium.org/8949026/diff/19001/net/spdy/spdy_frame_builder...
net/spdy/spdy_frame_builder.h:11: #include "base/sys_byteorder.h"
On 2011/12/28 00:50:20, eroman wrote:
> nit: order lexicographically.

Done.

[Nicolas Zea, 2011-12-28 21:05:08]

LGTM

[commit-bot: I haz the power, 2011-12-28 21:07:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8949026/27001

[commit-bot: I haz the power, 2011-12-28 23:18:25]

Change committed as 115926