Bug 98054 – Add htons/htonl definitions and implementations

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

Add htons/htonl definitions and implementations

[Joshua Bell]

Created attachment 166514 [details]
Patch

[Joshua Bell]

WIP - I need to update the xcode project to include the file.

In addition to bug 97794, the following files should be updated to use this:

platform/graphics/WOFFFileFormat.cpp
platform/graphics/chromium/VDMXParser.cpp

[Joshua Bell]

Created attachment 166518 [details]
Patch

[Adam Barth]

Comment on attachment 166518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166518&action=review

> Source/WTF/wtf/ByteOrder.h:39
> +namespace Internal {

Why not namespace WTF?

[Joshua Bell]

Comment on attachment 166518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166518&action=review

>> Source/WTF/wtf/ByteOrder.h:39
>> +namespace Internal {
> 
> Why not namespace WTF?

Since these helpers would only be defined on Windows and aren't intended to be used directly, WTF seemed like the wrong choice.

I looked around and found the 'namespace Internal' pattern used in several other headers in WTF/wtf, admittedly usually inside WTF. So I should probably change this to a nested namespace WTF { namespace Internal { ... } }

[Joshua Bell]

Created attachment 166521 [details]
Patch

[Joshua Bell]

Created attachment 166697 [details]
Patch

[Darin Adler]

Where will these be used? There are often better approaches to writing byte order independent code than adding htons calls, so I’d like to get a sense of where we want to use this and whether there is a superior idiom before adding it to WTF.

[Adam Barth]

Comment on attachment 166697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166697&action=review

> Source/WTF/WTF.xcodeproj/project.pbxproj:278
> +		EB95E1F0161A72410089A2F5 /* ByteOrder.h in Headers */ = {isa = PBXBuildFile; fileRef = EB95E1EF161A72410089A2F5 /* ByteOrder.h */; };

The project.pbxproj file is really bad for merge conflicts.  I tend to manually move these lines to be alphabetized by header name to help avoid conflicts (instead of everyone adding them to the end of the list)

[Joshua Bell]

(In reply to comment #9)
> 
> The project.pbxproj file is really bad for merge conflicts.  I tend to manually move these lines to be alphabetized by header name to help avoid conflicts (instead of everyone adding them to the end of the list)

It wasn't clear what the precedent was. Happy to re-order them.

(In reply to comment #8)
> Where will these be used? There are often better approaches to writing byte order independent code than adding htons calls, so I’d like to get a sense of where we want to use this and whether there is a superior idiom before adding it to WTF.

As mentioned in comment #2, htons (etc) are already used in:

platform/graphics/WOFFFileFormat.cpp
platform/graphics/chromium/VDMXParser.cpp

... and this was factored out of dependent bug:

https://bugs.webkit.org/show_bug.cgi?id=97794

Alternate suggestions appreciated.

[Joshua Bell]

Created attachment 166705 [details]
Patch

[Darin Adler]

Comment on attachment 166705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166705&action=review

If this really is giving us measurably better performance, than it seems fine to land it.

> Source/WTF/wtf/ByteOrder.h:61
> +#if CPU(BIG_ENDIAN)
> +#define ntohs(x) ((uint16_t)(x))
> +#define htons(x) ((uint16_t)(x))
> +#define ntohl(x) ((uint32_t)(x))
> +#define htonl(x) ((uint32_t)(x))
> +#elif CPU(MIDDLE_ENDIAN)
> +#define ntohs(x) ((unit16_t)(x))
> +#define htons(x) ((uint16_t)(x))
> +#define ntohl(x) WTF::Internal::wswap32(x)
> +#define htonl(x) WTF::Internal::wswap32(x)
> +#else
> +#define ntohs(x) WTF::Internal::bswap16(x)
> +#define htons(x) WTF::Internal::bswap16(x)
> +#define ntohl(x) WTF::Internal::bswap32(x)
> +#define htonl(x) WTF::Internal::bswap32(x)
> +#endif

Do these need to be macros rather than inline functions?

[Darin Adler]

Comment on attachment 166705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166705&action=review

> Source/WTF/wtf/ByteOrder.h:43
> +static inline uint32_t wswap32(uint32_t x) { return ((x & 0xffff0000) >> 16) | ((x & 0x0000ffff) << 16); }
> +static inline uint32_t bswap32(uint32_t x) { return ((x & 0xff000000) >> 24) | ((x & 0x00ff0000) >> 8) | ((x & 0x0000ff00) << 8) | ((x & 0x000000ff) << 24); }
> +static inline uint16_t bswap16(uint16_t x) { return ((x & 0xff00) >> 8) | ((x & 0x00ff) << 8); }

Including the static keyword is incorrect and non-helpful for inline functions defined in a header. Please just omit the “static” here.

[Joshua Bell]

Created attachment 166710 [details]
Patch

[Joshua Bell]

Created attachment 166711 [details]
Patch

[Joshua Bell]

(In reply to comment #12)
> 
> Do these need to be macros rather than inline functions?

No reason, other than inertia. Updated.

(In reply to comment #13)
> 
> Including the static keyword is incorrect and non-helpful for inline functions defined in a header. Please just omit the “static” here.

Thanks, fixed.

One more look?

[Alexey Proskuryakov]

Comment on attachment 166711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166711&action=review

> Source/WTF/wtf/ByteOrder.h:40
> +namespace Internal {

I'm not sure if we need WTF::Internal. Everything in WTF in internal unless you export it with a using declaration at the end of the file.

[Joshua Bell]

Comment on attachment 166711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166711&action=review

>> Source/WTF/wtf/ByteOrder.h:40
>> +namespace Internal {
> 
> I'm not sure if we need WTF::Internal. Everything in WTF in internal unless you export it with a using declaration at the end of the file.

I left that in as those functions are "internal" to the header; as they're only defined on OS(WINDOWS) I didn't want anything else in WTF to accidentally use them.

I take them out, though. Opinion?

[Benjamin Poulain]

Comment on attachment 166711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166711&action=review

> Source/WTF/ChangeLog:11
> +        For parsing or serializing binary data, byte order matters. The canonical htons/htonl/
> +        ntohs/ntohl functions are not present everywhere, so implementations are proliferating in
> +        parsers. Expose a new WTF header (wtf/ByteOrder.h) that includes the standard
> +        implementations on UNIX-like OSs and provides basic inlined implementations on Windows.

I would much prefer if you were fixing the other files along the addition of this header.

Without visibility on where it is gonna be used, it is impossible to know if this is the right solution.

[Joshua Bell]

Er, I *can* take them out...

(In reply to comment #19)
> 
> I would much prefer if you were fixing the other files along the addition of this header.
> 
> Without visibility on where it is gonna be used, it is impossible to know if this is the right solution.

Can do...

[Darin Adler]

Comment on attachment 166711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166711&action=review

> Source/WTF/wtf/ByteOrder.h:38
> +#if OS(WINDOWS)

I’d find this file easier to read if you left a blank line after this.

>>> Source/WTF/wtf/ByteOrder.h:40
>>> +namespace Internal {
>> 
>> I'm not sure if we need WTF::Internal. Everything in WTF in internal unless you export it with a using declaration at the end of the file.
> 
> I left that in as those functions are "internal" to the header; as they're only defined on OS(WINDOWS) I didn't want anything else in WTF to accidentally use them.
> 
> I take them out, though. Opinion?

Normally we only use WTF::Internal when Koenig lookup might expose them to programs by accident. Plain old WTF should be fine for these.

> Source/WTF/wtf/ByteOrder.h:46
> +} // namespace WTF
> +#if CPU(BIG_ENDIAN)

Should have a blank line between these two lines

> Source/WTF/wtf/ByteOrder.h:62
> +#endif // OS(WINDOWS)

I’d find this file easier to read if you left a blank line before this.

[Joshua Bell]

Created attachment 166777 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166777 [details]
Patch for landing

Clearing flags on attachment: 166777

Committed r130238: <http://trac.webkit.org/changeset/130238>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.