search

ikskuh / zig-network

A smallest-common-subset of socket functions for crossplatform networking, TCP & UDP
MIT License
484 stars 59 forks source link

refactor: General cleanup, alignment with `std` #86

Closed mochalins closed 4 months ago

mochalins commented 4 months ago

Reduce custom types and functions, preferring existing definitions in std. Standardize line lengths to idiomatic ~80~ 100 characters. Clean up of some TODO comments.

The socket function remains separate from the std.posix.socket implementation; however, the std implementation translates POSIX SOCK.NONBLOCK/SOCK.CLOEXEC to the Windows-specific implementation using ioctl, and thus there appears to be reasonable potential to integrate SOCK.DGRAM handling directly in std. This would enable using std.posix.socket directly.

Resolves #44.

mochalins commented 4 months ago

Edit: Just realized that actually the style guide suggests aiming for a line length of 100... I'll make adjustments here!

ikskuh commented 4 months ago

Can you please split that into two PRs, one that does the styleguide changes, and one that does the semantic changes?

Also can you explain why error.ConnectedTimedOut is equivalent to error.WouldBlock?

mochalins commented 4 months ago

Can you please split that into two PRs, one that does the styleguide changes, and one that does the semantic changes?

Will do!

Also can you explain why error.ConnectedTimedOut is equivalent to error.WouldBlock?

This was actually in the existing custom implementations of the Windows socket functions. AFAICT, the POSIX socket implementations return error.WouldBlock despite not configuring the sockets to be asynchronous and only specifying a timeout. In the same situations, the Windows sockets return error.ConnectedTimedOut, and thus I believe the existing implementation wrapped both error.WouldBlock and error.ConnectedTimedOut from Windows sockets to just error.WouldBlock.

As I am not deeply familiar with Windows sockets (and unfortunately do not currently have access to a Windows testing environment), I'm open to any feedback in regards to how this should change.

ikskuh commented 4 months ago

Okay, sounds reasonable. But can you move the error mapping code into a common function explaining this?

mochalins commented 4 months ago

Okay, sounds reasonable. But can you move the error mapping code into a common function explaining this?

In the semantic changes split out into #88, recvfrom is maintained as a separate Windows extern-wrapped function to avoid libc linking dependency, as the std.posix.recvfrom Windows implementation relies on an extern "c" fn recvfrom rather than extern "ws2_32" fn recvfrom.

In so doing, only one function connect now needs this error.ConnectedTimedOut -> error.WouldBlock handling, so I did not make a separate function. However, I did add a note explaining the behavior and also remarked that recvfrom uses similar handling.