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

[Experimental] WIP newtype redefinition of ProtocolNumber #469

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

archaephyrryx
Copy link
Contributor

redefines ProtocolNumber as newtype around CInt (formerly type alias for CInt)
that re-derives all instance methods for CInt that are defined in Foreign.C.Types

defines and exports pattern synonyms for commonly used protocol-number constants (defined in "netinet/in.h")
(IPPROTO_(IP|IPV4|IPV6|TCP|UDP|ICMP|ICMPV6|RAW)) as well as UnsupportedProtocol and GeneralProtocol patterns

refactored function definitions previously relying on (ProtocolNumber ~ CInt) to use unwrapper function

implements bijective read/show instances for ProtocolNumber whose default behavior is to directly read and show integer values with no constructor syntax

redefines ProtocolNumber as newtype around CInt (formerly type alias for CInt)
that re-derives all instance methods for CInt that are defined in Foreign.C.Types

defines and exports pattern synonyms for commonly used protocol-number constants (defined in "netinet/in.h")
(IPPROTO_(IP|IPV4|IPV6|TCP|UDP|ICMP|ICMPV6|RAW)) as well as UnsupportedProtocol and GeneralProtocol patterns

refactored function definitions previously relying on (ProtocolNumber ~ CInt) to use unwrapper function

implements bijective read/show instances for ProtocolNumber whose default behavior is to directly read and show integer values with no constructor syntax
@archaephyrryx
Copy link
Contributor Author

This patch constitutes an API change that would potentially break network-dependent packages depending on how reliant they are on the reflection ProtocolNumber ~ CInt. It may require a code-survey of network-dependent packages before it can be safely merged without breaking a lot of infrastructure.

Unbreaks doctest by substituting new `show` value of defaultProtocol
where it appears in haddock examples

Clarifies behavior of show for ProtocolNumber to highlight that
pattern synonym names and their corresponding show values are based
on assumption of IP protocol families even though the type itself
remains general over all protocol families.
@archaephyrryx
Copy link
Contributor Author

archaephyrryx commented Jul 17, 2020

I also noticed that an incomplete pattern warning for Network/Socket/Types.hsc can be fixed by replacing

isSupportedFamily :: Family -> Bool
isSupportedFamily f = case f of
    UnsupportedFamily -> False
    GeneralFamily _   -> True

with

isSupportedFamily :: Family -> Bool
isSupportedFamily f = case f of
    UnsupportedFamily -> False
    _                 -> True

without otherwise changing the behavior of the function.

This change is trivial and safe enough that it would probably be better to directly fix it on master than tie a commit to this PR.

Redefines ProtocolNumber synonyms in terms of canonical "magic" numbers
from IANA reference (https://www.iana.org/assignments/protocol-numbers)
rather than CPP-constants from C header file.

Removes UnsupportedProtocol as all definitions are unconditional instead
of relying on C header-file macro definitions; redefines test case to
ensure that the value 'ProtocolNumber (-1)' is represented shown as "-1"
rather than "UnsupportedProtocol"

Renames IPPROTO_IP (dummy for '0') as DefaultProtocol, both as a pattern
synonym and in read/show boilerplate bijection (as well as in haddock
examples and tests/Network/SocketSpec.hs)
@archaephyrryx
Copy link
Contributor Author

I am currently trying to assess how big an impact this API change would have on existing libraries, and have also requested an external reviewer independently. For now, I don't think this is ready to merge yet, but there is not much more to be done for it besides a few minor tweaks assuming everything looks good.

@kazu-yamamoto
Copy link
Collaborator

@archaephyrryx Since v3.1.2.0 was released, I think it's time to tackle this PR. Do you think that this PR is ready for merging?

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.

2 participants