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 Crystal::System::Addrinfo #14957

Conversation

HertzDevil
Copy link
Contributor

Moves the platform-specific code into a separate module, so that implementations other than LibC.getaddrinfo can be added without cluttering the same file (e.g. Win32's GetAddrInfoExW from #13619). Depends on #14956.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

What about moving the yielding getaddrinfo(&) as a system method? That would abstract the next/free logic.

@straight-shoota
Copy link
Member

straight-shoota commented Sep 4, 2024

thought: Maybe getaddrinfo(&) should actually be pushed down entirely to the implementation level. Both current implementations work by iterating abstract handles. But that doesn't have to be the case for all implementations.
It's fine to leave it as is for now, though. We can adapt when the need arises.

@straight-shoota
Copy link
Member

thought: I'm wondering if it really makes sense to split the getaddrinfo implementations for Unix and Windows.
The function is almost identical between POSIX and WinSock. So we have a lot of duplication in the handling code. IMO the merged code was actually quite readable with some target-specific conditionals here and there.

@HertzDevil
Copy link
Contributor Author

The function is almost identical between POSIX and WinSock

Don't worry, GetAddrInfoExW will look different enough

@straight-shoota
Copy link
Member

So the plan is to switch entirely to GetAddrInfoExW in a follow-up? Sounds great, then!

@ysbaddaden
Copy link
Contributor

My intent was for the system API to be the yielding getaddrinfo(&) and skip the next/free methods + passing an object (internal détail). Each system implementation will be free to iterate the resolved addresses as they need.

@straight-shoota straight-shoota added this to the 1.14.0 milestone Sep 4, 2024
@straight-shoota straight-shoota merged commit 7776438 into crystal-lang:master Sep 5, 2024
63 of 65 checks passed
@HertzDevil HertzDevil deleted the refactor/crystal-system-addrinfo branch September 6, 2024 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants