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

optval pointer fix #61

Closed
wants to merge 1 commit into from
Closed

optval pointer fix #61

wants to merge 1 commit into from

Conversation

reqshark
Copy link
Collaborator

@nickdesaulniers, following up a discussion we had about getsockopt args in #46:

The third arg, optval, should be a void*. Passing a int [64] is probably not what you meant to do?

I'm pretty sure NAN or maybe node V8 is partial to char* over void*... though i guess it depends on the situtation/function we're talking about

This does work, it's just using 63 * sizeof(int) bytes more of the stack than it needs to. It works because in C arrays decay into pointers when passed to functions. So you're allocating space for 64 ints on the stack, then passing the address of the first element, which gets filled. Since it's an array of ints, only the first element is ever populated.

If you think about the C ABI, it's not possible to represent 64 ints in a single register on today's processors. That's why passing an array decays into a pointer. A kind of syntactic sugar. nn_getsockopt takes a void_, which works since optval decays into an int_.

A few lines down, there is no need to access optval[0] since we don't need optval to be an array of ints, just an int.

agreed.

following up a discussion we had about getsockopt in #46
@nickdesaulniers
Copy link
Owner

I'm sorry, this is not right. I will fix this when I get a chance, but the gist is we have to do something special in the case of pub sub. Se also #31 and #37.

@nickdesaulniers
Copy link
Owner

See also how get and set socket option is used within nanomsg/nanomsg. The gist is for some options we need one single integer (no arrays) (for pretty much every option) or a character array (for pub sub).

@reqshark
Copy link
Collaborator Author

oh ok, is there string use other than subscription keys/channels?

@nickdesaulniers
Copy link
Owner

I don't think so, but I need to re analyze nanomsg/nanomsg to triple check.

@reqshark reqshark deleted the intstar branch March 9, 2015 20:33
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.

None yet

2 participants