-
-
Notifications
You must be signed in to change notification settings - Fork 217
Fix Nan type missmatch warnings #89
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
Conversation
| if (!info[0]->IsNumber()) | ||
| return Nan::ThrowTypeError("Option must be an integer"); | ||
| int64_t option = Nan::To<int64_t>(info[0]).FromJust(); | ||
| int option = Nan::To<int>(info[0]).FromJust(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should GetSockOpt accept int64_t?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well outside my expertise. 🙇 @minrk any hunches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few int64 options. Hopefully The docs cover it. pyzmq handles it by keeping track of a list of options that should be int64, int, bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice approach in pyzmq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@minrk Oh I see, thanks for the links.
Would it be better to keep using int64_t and change GetSockOpt and SetSockOpt to use int64_t there as well?
My C++ knowledge is rather limited 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I re-read, I think I see what's going on here and int64_t should be left. This is using the same 64b of memory for all the numerical versions of GetSockOpt, but casting that address to other (potentially smaller) types based on the expected type of the option.
Since int and int64 are the same size on a 64b build, there's no problem switching. However, you should get segfaults on 32b systems, because int is only 32b and the explicit int64 and uint64 options (L667, 669 below) will try to write 64b to 32b of memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@minrk Thanks for the clarification 👍
I'll make a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg, sorry. Now that I've read it again, this is only option, which should be an int not int64, and not optval, the result which has varying type. Everything is fine here, move along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better 👍
Thanks for taking a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta love C++ and type conversion.
Current coverage is 91.16% (diff: 100%)
|
|
@n-riesco Thanks for the link! If I read correctly this fix should be OK. |
I wrestled down some windows build warnings 😄
The only warning I don't know how to fix is thrown here: