Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversions between HostAddress(6) and endianness-independent tuples #210

Merged
merged 7 commits into from
Jul 26, 2016
Merged

Conversions between HostAddress(6) and endianness-independent tuples #210

merged 7 commits into from
Jul 26, 2016

Conversation

ppetr
Copy link
Contributor

@ppetr ppetr commented Jul 2, 2016

.. to allows direct, more convenient manipulation with low-level IP addresses.

@eborden
Copy link
Collaborator

eborden commented Jul 8, 2016

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.

@ppetr
Copy link
Contributor Author

ppetr commented Jul 9, 2016

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

Dne pá 8. 7. 2016 20:36 uživatel Evan Borden notifications@github.com
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
.

@kazu-yamamoto
Copy link
Collaborator

kazu-yamamoto commented Jul 19, 2016

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

@vincenthz
Copy link

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)

@kazu-yamamoto
Copy link
Collaborator

@vincenthz Thank you for the information!

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

@ppetr
Copy link
Contributor Author

ppetr commented Jul 21, 2016

@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.)

@ppetr
Copy link
Contributor Author

ppetr commented Jul 24, 2016

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.

@@ -1021,7 +1027,10 @@ aNY_PORT = 0
iNADDR_ANY :: HostAddress
iNADDR_ANY = htonl (#const INADDR_ANY)

-- | Converts the from host byte order to network byte order.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to enumerate the possible id behaviour of these functions.

@kazu-yamamoto kazu-yamamoto merged commit 3fd4903 into haskell:master Jul 26, 2016
@kazu-yamamoto
Copy link
Collaborator

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

@ppetr ppetr mentioned this pull request Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants