[webkit-reviews] review granted: [Bug 98054] Add htons/htonl definitions and implementations : [Attachment 166711] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 98054: Add htons/htonl definitions and implementations
https://bugs.webkit.org/show_bug.cgi?id=98054

Attachment 166711: Patch
https://bugs.webkit.org/attachment.cgi?id=166711&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.