Skip to content

Conversation

marler8997
Copy link
Contributor

No description provided.

@marler8997 marler8997 force-pushed the shutdownSockets branch 4 times, most recently from a0563e5 to b1cafa8 Compare November 28, 2020 06:58
@LemonBoy
Copy link
Contributor

You may be interested in #7194 and the sibling #7124

@marler8997 marler8997 marked this pull request as ready for review November 28, 2020 07:22
@marler8997 marler8997 force-pushed the shutdownSockets branch 2 times, most recently from 1cc51e6 to 46bcd72 Compare November 28, 2020 13:34
}

pub fn shutdown(fd: i32, how: i32) usize {
pub fn shutdown(fd: i32, how: c_int) usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

Native zig types are preferred, why this change?

Copy link
Contributor Author

@marler8997 marler8997 Nov 28, 2020

Choose a reason for hiding this comment

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

ok I've removed this change and modified the new declaration in c.zig to use i32 as well.

lib/std/os.zig Outdated
SystemResources
} || UnexpectedError;

pub const ShutdownHow = enum {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this a simple Zig enum enum { read_end, write_end, both_ends } and convert this to the proper flag value in fn shutdown.

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 dunno, this seems simpler than adding conversion logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a leaky abstraction. The "conversion" code is a simple switch, the same you're doing here with a lot of ifs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I've changed it to switches internal to the function. This seems like it could be adding unnecessary overhead to me, but not a big deal.

@marler8997 marler8997 force-pushed the shutdownSockets branch 4 times, most recently from 4d4eddc to 2bca2e4 Compare November 28, 2020 20:34
lib/std/c.zig Outdated
pub extern "c" fn uname(buf: *utsname) c_int;

pub extern "c" fn gethostname(name: [*]u8, len: usize) c_int;
pub extern "c" fn shutdown(socket: fd_t, how: i32) usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

for how, I would use a c_int instead of i32. c_int could have different size depending the platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see @LemonBoy said the opposite before for lib/std/os/linux.zig function.

@LemonBoy, using c_int ensure portability: C standard (on which shutdown() POSIX is based) defines no size of int (only a minimum range, and relative size compared to short and long)

Copy link
Contributor

Choose a reason for hiding this comment

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

and I think also the return value is wrong: usize is unsigned, whereas POSIX shutdown() could return -1 on error. I would use c_int here too 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see @LemonBoy said the opposite before for lib/std/os/linux.zig function.

That's the Linux syscall wrapper, we're directly talking with the kernel there and don't give a damn about C/libc, the types there should be Zig's native ones.

The c.zig definitions, on the other hand, should use the C types instead to guarantee the ABI compatibility.

and I think also the return value is wrong: usize is unsigned, whereas POSIX shutdown() could return -1 on error. I would use c_int here too

Good catch, c_int is the correct return type.

Copy link
Contributor Author

@marler8997 marler8997 Nov 29, 2020

Choose a reason for hiding this comment

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

Ok I've modified the one in c.zig to:

pub extern "c" fn shutdown(socket: fd_t, how: c_int) c_int;

@andrewrk andrewrk merged commit b587a42 into ziglang:master Nov 30, 2020
@marler8997 marler8997 deleted the shutdownSockets branch December 2, 2020 05:59
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.

4 participants