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

Factor out toAddrinfo helper #49

Merged
merged 1 commit into from
Sep 13, 2022
Merged

Factor out toAddrinfo helper #49

merged 1 commit into from
Sep 13, 2022

Conversation

armanbilge
Copy link
Owner

Very sorry to interrupt IPv6 work @LeeTibbert ...

This PR factors out the two uses of getaddrinfo into a single, shared method. This should reduce the amount of changes necessary for IPv6. I hope it helps you more than it distracts.

@armanbilge armanbilge marked this pull request as ready for review September 12, 2022 15:39
@LeeTibbert
Copy link
Collaborator

Thank you for the update.
I have been working on the IPv6 stuff all day.
I have caught a couple of places which were not evident.

Whilst ratting around, I noticed the opportunity to factor
out some of the getaddrinfo stuff. A little of the handling
differs between the two.

I'll take a walk, switch context, clear my mind, and come
back to review.

I've been trying to find a bit of tricky code that I know that I
wrote no more than a week ago, no luck. Arrgh!

Thank you for doing this refactor.

@LeeTibbert
Copy link
Collaborator

LGTM, thank you for the opportunity to review.

  1. I triple checked the "rtn == 0" in one file and "rtn != 0" in the other.
    Looks like you got rid of that incongruity and the replacement
    looks correct to me.

  2. An added benefit is that you removed the non-local return
    that I have been whistling past for a while. That heads
    off SN 3.2.0 quite vocal warnings.

Copy link
Collaborator

@LeeTibbert LeeTibbert left a comment

Choose a reason for hiding this comment

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

LGTM -- looks correct and much easier to follow.

@armanbilge
Copy link
Owner Author

Thanks, I'm glad you liked it. Sincere apologies for any merge conflicts I am about to create for you :(

@armanbilge armanbilge merged commit 212d44e into main Sep 13, 2022
@armanbilge armanbilge deleted the pr/refactor-toaddrinfo branch September 13, 2022 01:10
@LeeTibbert
Copy link
Collaborator

Sincere apologies for any merge conflicts I am about to create for you :(

Thank you for your concern & consideration. Life on the bleeding edge!

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