I don't have a strong opinion on the introduction of these functions, but I would like to see tests provided for this code, either in the suite or in doctest.

@kazu-yamamoto Might have a stronger opinion.

Good point, I'll add proper tests and update the PR.

Dne pá 8. 7. 2016 20:36 uživatel Evan Borden [email protected]
napsal:

I don't have a strong opinion on the introduction of these functions, but
I would like to see tests provided for this code, either in the suite or in
doctest.

@kazu-yamamoto https://github.com/kazu-yamamoto Might have a stronger
opinion.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#210 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAJ89SMpdfSiLy0GMIO4Cs-ZQK_HtMlKks5qTpixgaJpZM4JDo_a
.

I don't have strong opinions and can accept this pull request. Let's merge when ready.

There's really no case for exporting the C functions htons, htonl, ntohs, ntohl through network. They are already available in Data.Word as byteSwap16 and byteSwap32 (>= ghc 7.8)

@vincenthz Thank you for the information!

So, I think what we should do is documentation to suggest to use these functions if necessary.

@vincenthz This is not entirely correct. Functions byteSwap... swap byte order unconditionally. But htonl/s and ntohl/s swap between host and network endianness. This means that on BE hosts they're id and on LE hosts they're byteSwap.... So without them, other means of detecting host endianness is needed and usage of byteSwap... becomes awkward.

Therefore I still believe that exporting hton... and ntoh... is the easiest and most convenient approach.

(Also the exports is only a part of this PR. Regardless of them, the other functions for manipulating IP addresses as tuples of bytes/words are still quite useful, as they provide access to addresses as they're defined in the specification, rather than how they're represented in memory as certain Words.)

I added basic tests for the new functions. Comments welcome. It'd be slightly cleaner to add a QuickCheck test, but the difference isn't that big and I wasn't sure if you'd be OK with adding a new dependency.

I have merged this pull request. Thank you for your contribution.