Bug 98054 - Add htons/htonl definitions and implementations
Summary: Add htons/htonl definitions and implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 97794
  Show dependency treegraph
 
Reported: 2012-10-01 11:42 PDT by Joshua Bell
Modified: 2012-10-02 17:31 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.25 KB, patch)
2012-10-01 11:46 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2012-10-01 11:52 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2012-10-01 12:08 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2012-10-02 09:14 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (9.94 KB, patch)
2012-10-02 10:12 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (10.28 KB, patch)
2012-10-02 10:34 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2012-10-02 10:38 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (12.73 KB, patch)
2012-10-02 16:41 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-10-01 11:42:00 PDT
Add htons/htonl definitions and implementations
Comment 1 Joshua Bell 2012-10-01 11:46:25 PDT
Created attachment 166514 [details]
Patch
Comment 2 Joshua Bell 2012-10-01 11:48:59 PDT
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
Comment 3 Joshua Bell 2012-10-01 11:52:29 PDT
Created attachment 166518 [details]
Patch
Comment 4 Adam Barth 2012-10-01 12:01:13 PDT
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?
Comment 5 Joshua Bell 2012-10-01 12:04:19 PDT
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 { ... } }
Comment 6 Joshua Bell 2012-10-01 12:08:06 PDT
Created attachment 166521 [details]
Patch
Comment 7 Joshua Bell 2012-10-02 09:14:48 PDT
Created attachment 166697 [details]
Patch
Comment 8 Darin Adler 2012-10-02 10:04:59 PDT
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.
Comment 9 Adam Barth 2012-10-02 10:06:19 PDT
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)
Comment 10 Joshua Bell 2012-10-02 10:08:18 PDT
(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.
Comment 11 Joshua Bell 2012-10-02 10:12:12 PDT
Created attachment 166705 [details]
Patch
Comment 12 Darin Adler 2012-10-02 10:14:18 PDT
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?
Comment 13 Darin Adler 2012-10-02 10:14:46 PDT
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.
Comment 14 Joshua Bell 2012-10-02 10:34:58 PDT
Created attachment 166710 [details]
Patch
Comment 15 Joshua Bell 2012-10-02 10:38:20 PDT
Created attachment 166711 [details]
Patch
Comment 16 Joshua Bell 2012-10-02 10:39:19 PDT
(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?
Comment 17 Alexey Proskuryakov 2012-10-02 16:12:19 PDT
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.
Comment 18 Joshua Bell 2012-10-02 16:20:40 PDT
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?
Comment 19 Benjamin Poulain 2012-10-02 16:20:52 PDT
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.
Comment 20 Joshua Bell 2012-10-02 16:22:08 PDT
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...
Comment 21 Darin Adler 2012-10-02 16:30:08 PDT
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.
Comment 22 Joshua Bell 2012-10-02 16:41:05 PDT
Created attachment 166777 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-10-02 17:31:13 PDT
Comment on attachment 166777 [details]
Patch for landing

Clearing flags on attachment: 166777

Committed r130238: <http://trac.webkit.org/changeset/130238>
Comment 24 WebKit Review Bot 2012-10-02 17:31:19 PDT
All reviewed patches have been landed.  Closing bug.