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

Add IP address classes to net package #158

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Jan 7, 2020

and return them from NetAddress instead of their raw representation.

Rendered

Fun fact: Making the IPv6Address class Hashable was the actual motivation for #157

@mfelsche mfelsche changed the title Add RFC for adding IP address classes Add IP address classes to net package Jan 7, 2020
@SeanTAllen
Copy link
Member

During sync, I asked @mfelsche to update this with an example of what is currently problematic about using the current interface.

@mfelsche
Copy link
Contributor Author

mfelsche commented Jan 7, 2020

I am gonna update this RFC to not break the current 'NetAddress' methods.

@mfelsche
Copy link
Contributor Author

I updated this PR and deliberately decided for introducing a breaking change in NetAddress. It just didn't seem reasonable to still return the raw representation from NetAddress.ipv4_addr() and not the new classes. Instead I rooted for introducing new methods: fun ipv4_addr_raw(): U32 and fun ipv6_addr_raw(): (U23, U32, U32, U32).

@SeanTAllen
Copy link
Member

I would like to see

"How we teach this" and "How we test this" expanded on. They feel very handwavey at this moment. What specifically should be tested? What specifically should be documented? Should we be updating any package level docs with documentation? Should we add something to any of the programs in examples? Perhaps add some examples?

@SeanTAllen
Copy link
Member

I don't really know what I am signing up for with the detailed design. You appear to have a solid idea. I couldn't implement this based on the detailed design. Do you have a more solid idea of what you would do? Can you add more Pony code to demonstrate?

@SeanTAllen
Copy link
Member

From the RFC, I don't understand the point below. Why is this an alias? You seem to be talking about something that sounds like an interface to me. Can you provide some example code of what you mean and how it would be used?

A type alias IPAddress will be created:

type IPAddress is (IPv4Address | IPv6Address)

this way it is possible to handle a generic IPAddress using the methods common to both IPv4Address. and IPv6Address. It might be a valid alternative to use an interface IPAddress instead.


# Drawbacks

* Making use of an IP address requires an additional object allocation.
Copy link
Member

Choose a reason for hiding this comment

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

How likely are such object allocations? I think we need to do more than note that they will happen. Do we expect that this code end up being used in hot code path? IE, will this happen a lot? If yes, this is a huge drawback. If no, then its probably minor.

# Drawbacks

* Making use of an IP address requires an additional object allocation.
* Breaks existing code with regards to `NetAddress`.
Copy link
Member

Choose a reason for hiding this comment

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

I think the RFC should document what changes folks will have to do to make the change. This will better help us understand the extent of the drawback.


# Alternatives

Create primitives for IP address handling that work directly on the raw representations `U32` for IPv4 and `U128` for IPv6 for which type aliases can be created. This way no allocations are needed, but we have a very non-OO and non-intuitive API and representation.
Copy link
Member

Choose a reason for hiding this comment

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

I don't find non-OO to be an issue. The number 1 Pony Philosophy point is "performance", not "OO". I also don't see how the API would be "non-intuitive". Can you expand on what would be "non-intutitive" about it? Examples would favorable.

Given the option of "no allocations", I will want no allocations unless you can show me why the drawbacks outweigh the removal of said allocations.


# Unresolved questions

Should the way for handling a generic IP address (IPv4 or IPv6) be via a type-union or an interface?
Copy link
Member

Choose a reason for hiding this comment

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

How is this unresolved? The detailed design says type union not interface. Seems like this is more of an alternative than an unresolved question.

@SeanTAllen SeanTAllen added status - final comment period The RFC is finalized. Waiting for final comments. status - ready for vote The RFC is ready to be voted on. and removed status - final comment period The RFC is finalized. Waiting for final comments. status - ready for vote The RFC is ready to be voted on. labels May 25, 2020
@SeanTAllen SeanTAllen added status - ready for vote The RFC is ready to be voted on. status - final comment period The RFC is finalized. Waiting for final comments. and removed status - final comment period The RFC is finalized. Waiting for final comments. status - ready for vote The RFC is ready to be voted on. labels May 25, 2020
Base automatically changed from master to main February 8, 2021 22:15
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