Change NetAddress class to hide its fields behind functions, fixing cross-platform compatibility.

[aburn]

made existing fields private. and exposed utility functions to access…… the fields. Platform specific information is handled in these utility functions.

[jemc]

Thanks for working on this!

I've left some small code style comments.

[jemc]

I think the one place where this type alias is used doesn't really warrant adding another name to the namespace. I think this line should be removed, and the line below that uses it should just use (U32 | Array[U32]) directly

[jemc]

The end here should be the same level of indentation as the if that it terminates. That is, it should be indented one more level to the right.

[jemc]

I think this method should probably have a docstring to explain exactly what it returns in the (U32 | Array[U32]). That is, it should explain what those 32-bit integers mean in each case.

[jemc]

This red icon is GitHub's way of saying that this file is missing an empty line at the end, which is a common convention for file endings. Adding an empty line at the end here will make this nag icon go away.

[jemc]

If we want to expose this for FFI use in the standard library, it needs to get either a pony_ or ponyint_ prefix to its name, for cleanliness of the function namespace. pony_ means it is a public API meant for use by third party code, whereas ponyint_ means it is internal. Unless there is a good reason to make it public, we should prefer to keep it internal because public functions require us to treat any changes to them as breaking changes in Pony.

So, this function should end up named ponyint_address_length, or something else with that prefix.

[mfelsche]

I left some inline comments. In general very promising first iteration.

[mfelsche]

what do you think about creating two different methods, on for the IPv4 and one for the IPv6 address?
The user has to check what kind of address this is anyways, with a method able to return each, a second match statement necessary, like so:

let addr: NetAddress = ... // somehow get a  NetAddress
if addr.ip4() then
  match addr.addr()
  |  let ip4: U32 => // do something with ip4 address
  else
     // somehow handle this actually impossible case, otherwise this would always return something or None
  end

The as statement is also an alternative here. But after all, it might be more straightforward to do:

let addr: NetAddress = ... // somehow get a  NetAddress
if addr.ip4() then
  let ip4: U32 = addr.ip4_addr()
  // do something with ip4
end

It would need to be documented that the return value of a fun ip4_addr(): U32 method is actually invalid if ip4() is not true. This would basically repeat the behavior we currently have with only the fields.

[mfelsche]

Also each returned integer value (except U8 of course) in any of these methods should be run through a method to convert between network and host byte order. These are ntohl for U32 and ntohs for U16.

[mfelsche]

please indent the function body with 2 spaces according to the style guide.

[mfelsche]

please use @noths[U16](_port) here, to convert to host byte order.

[mfelsche]

please use @nothl[U32](_scope) here, to convert to host byte order.

[mfelsche]

what do you think about returning a 4-tuple here instead? Just a thought, Array[U32] is also fine.

[jemc]

I agree that a 4-tuple is a better idea - the number of elements is a constant known at compile time, so a tuple is a better representation of that. Array[U32] is more cumbersome because your code has to deal with the possibility of the array being more or fewer than 4 elements, which we know is impossible, but the compiler won't know it.

[mfelsche]

you can use parts of the port field docstring here.

[mfelsche]

you can move the scope field docstring here.

[mfelsche]

Everything looks good, i am just not 100% sure about this check. Could we have a testcase that verifies that the methods actually return what they should?

[SeanTAllen]

I think the change log should be changed rather than fixed

[SeanTAllen]

This is a breaking change, yes? If yes, before this is merged, can someone please supply release notes that show what code used to look like, what sort of error a user should expect and what they need to do to get their code working?

[jemc]

I think the changelog entry belongs in both sections, which is why I added both tags and updated the PR name to make sense in both contexts.

[SeanTAllen]

Marking this as "DO NOT MERGE". Can be removed once release notes are added.

[mfelsche]

Release notes draft

net.NetAddress has been modelled after the C union sockaddr_storage which happens to have different fields on different platforms (i.e. on linux a 16 bit family field and on e.g. OSX an 8 bit length field an only a 8 bit family field). This PR unifies net.NetAddress to return consistent values on any platform Pony is supported on. To this end, all previously public fields on this class have been made private and new functions have been introduced to return those values:

• Removed Field | New Method
• length: U8 | length(): U8
• family: U8 | family(): U8
• port: U16 | port(): U16
• addr: U32 | ipv4_addr(): U32
• addr1, addr2, addr3, addr4 | ipv6_addr(): (U32, U32, U32, U32)
• scope: U32 | scope(): U32

All functions return the values in host byte order, that means conversion using @ntohX FFI functions is not necessary anymore. Before calling and using ipv4_addr() or ipv6_addr() check ip4(): Bool and ip6(): Bool which one to use, otherwise the returned values will be invalid (as before using the fields).