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

src: remove __builtin_bswap16 call #4290

Merged
merged 2 commits into from
Dec 15, 2015
Merged

Conversation

bnoordhuis
Copy link
Member

Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap. If it doesn't, fix the compiler.

Fixes: #4284

CI: https://ci.nodejs.org/job/node-test-pull-request/1002/

@cjihrig
Copy link
Contributor

cjihrig commented Dec 15, 2015

CI is fine. LGTM.

@Fishrock123 Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 15, 2015
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: nodejs#4290
Reviewed-By: Colin Ihrig <[email protected]>
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: nodejs#4284
PR-URL: nodejs#4290
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

@bnoordhuis should this be backported to LTS? or is this making changes to stuff that never made it to 4.x

@MylesBorins
Copy link
Contributor

@Fishrock123 we should add this to #4281

@bnoordhuis
Copy link
Member Author

@thealphanerd Added lts-watch-v4.x label. Didn't see it in v4.x but it's still in v4.x-staging.

bnoordhuis added a commit that referenced this pull request Dec 15, 2015
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: #4284
PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
@bnoordhuis
Copy link
Member Author

That's commit 2ccde01 from #3410.

@MylesBorins
Copy link
Contributor

Awesome thanks!

I wonder if there is a smart way we can modify the meta data of commits landed on LTS to specify that other commits must land with them. I'm going to mull on this and likely open an issue about it if I can think of a solution

bnoordhuis added a commit that referenced this pull request Dec 15, 2015
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
bnoordhuis added a commit that referenced this pull request Dec 15, 2015
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: #4284
PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
@rvagg rvagg mentioned this pull request Dec 17, 2015
@MylesBorins
Copy link
Contributor

#3410 will have to land into LTS before this commit can

MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: #4284
PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: #4284
PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: #4284
PR-URL: #4290
Reviewed-By: Colin Ihrig <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Remove the unused and broken BITS_PER_LONG macro.  Broken because x64
is the only 64 bits architecture where it produces the right result.

PR-URL: nodejs#4290
Reviewed-By: Colin Ihrig <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Not supported by apple-gcc and I'm not convinced it's worth adding more
preprocessor hacks when it should be easy as pie for the compiler to
to optimize the byteswap.  If it doesn't, fix the compiler.

Fixes: nodejs#4284
PR-URL: nodejs#4290
Reviewed-By: Colin Ihrig <[email protected]>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jul 11, 2016
Said builtins are not supported by older versions of apple-gcc, breaking
the build on OS X 10.8.

Fixes: nodejs#7618
Refs: nodejs#4290
Refs: nodejs#7157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang 4.2 breakage, Node 5.2.0
5 participants