-
Notifications
You must be signed in to change notification settings - Fork 30k
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
src: fix process.platform
#271
Conversation
#endif | ||
READONLY_PROPERTY(process, "platform", platform); | ||
READONLY_PROPERTY(process, "platform", | ||
OneByteString(env->isolate(), NODE_PLATFORM)); |
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.
Style: indentation is off here.
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.
@bnoordhuis what is the preferred way? Like this: https://github.com/iojs/io.js/blob/v1.x/src/node.cc#L2655 or like this https://github.com/iojs/io.js/blob/v1.x/src/node.cc#L2666 ?
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.
The former is the preferred style. The latter is for when the former would exceed 80 columns.
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.
fixed
e1fe270 introduces NODE_PLATFORM macro which had to be redefined in node.gyp for `process.platform` to return expected values.
66cc909
to
6bbdde8
Compare
LGTM. I'll let @piscisaureus sign off on it and land it. |
@vkurchatkin And thanks, of course! :-) |
It was kind of a dumb mistake of me to assume that windows would be the only platform where the node platform was different from the gyp Thanks @vkurchatkin for fixing this! Landed in 9f45799. |
e1fe270 introduces NODE_PLATFORM macro which had to be redefined in node.gyp for
process.platform
to return expected values.Not sure about win and solaris (can't test), but for OS X
process.platform
returnedmac
instead ofdarwin
and broke some tests./cc @piscisaureus