network icon indicating copy to clipboard operation
network copied to clipboard

[Experimental] WIP newtype redefinition of ProtocolNumber

Open archaephyrryx opened this issue 4 years ago • 4 comments

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 avatar Jul 17 '20 17:07 archaephyrryx

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.

archaephyrryx avatar Jul 17 '20 17:07 archaephyrryx

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.

archaephyrryx avatar Jul 17 '20 19:07 archaephyrryx

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.

archaephyrryx avatar Jul 19 '20 21:07 archaephyrryx

@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?

kazu-yamamoto avatar Sep 18 '20 06:09 kazu-yamamoto