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

std/socket: Use 0 as default protocol number #1265

Draft
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

iacore
Copy link
Contributor

@iacore iacore commented Apr 2, 2024

Summary

  • The default protocol number is changed to 0, as specified by POSIX.
  • We are changing it to align the standard library to POSIX standards.

Notes for Reviewers

Prior as nim-lang/Nim#19674.

I don't know what I am doing. I only know that the default protocol number should be 0 (default protocol for AF & socket type, to be determined by the OS).

nativesocket.nim has a lot of duplicated code that could be simplified with meta-programming techniques. We should clean that up.

@iacore iacore marked this pull request as ready for review January 18, 2025 06:38
@iacore
Copy link
Contributor Author

iacore commented Jan 18, 2025

I forgot to mark this branch as ready for review.

@saem
Copy link
Collaborator

saem commented Jan 18, 2025

it doesn't pass ci and the pr message isn't even attempted at, this isn't ready for review.

@saem saem marked this pull request as draft January 18, 2025 06:57
@iacore
Copy link
Contributor Author

iacore commented Jan 18, 2025

Is it now ready for review?

@zerbina
Copy link
Collaborator

zerbina commented Jan 18, 2025

Please elaborate on why you want to make this change. nativesockets doesn't claim to adhere to the POSIX standard, and I also don't see why it should.

@iacore
Copy link
Contributor Author

iacore commented Jan 21, 2025

If I want to use UDP, the default protocol number is wrong. Berkeley socket API says default protocol should be 0.

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Can you describe not just the change, but what's the reasoning for the change. The net module is meant for cross-platform socket interface, posix alone isn't that, so simply saying that's what it recommends doesn't make sense across all platforms, at least at first glance.

I don't need fancy prose, feel free to use bullet points or whatever makes it easier. But at this point I'd have to do a deep dive across platforms and the various APIs, refresh my understanding the module design as it is today, then reason through the changes made and guess as to what you're thinking. If that's what you're expecting I don't see most people having time to do that any time soon.

sol-nimskull.nim Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why it ended up there

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.

3 participants