/node

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 NODE_DEPRECATED macro with old compilers #1626

Merged
merged 1 commit into from May 5, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
C++
Projects
None yet
Milestone
No milestone
6 participants
@bnoordhuis @rvagg @trevnorris @tojocky @silverwind @mscdex
@bnoordhuis
Member

bnoordhuis commented May 5, 2015

The __attribute__((deprecated("warning"))) macro didn't exist until
gcc 4.5 and clang 2.9.

While io.js does not build with compilers that old, add-ons do. Let's
make src/node.h compatible with such compilers, it's a public header.

Refs: #1619

R=@trevnorris?
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis May 5, 2015

Member

/cc @misterdjules
Member

bnoordhuis commented May 5, 2015

/cc @misterdjules
@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg May 5, 2015

Member

lgtm, I think, perhaps R=@koopa
Member

rvagg commented May 5, 2015

lgtm, I think, perhaps R=@koopa
@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris May 5, 2015

Contributor

I'd like to see the \ pushed out to the end, style-wise, but LGTM.
Contributor

trevnorris commented May 5, 2015

I'd like to see the \ pushed out to the end, style-wise, but LGTM.

@mscdex mscdex added build C++ labels May 5, 2015

@bnoordhuis
src: fix NODE_DEPRECATED macro with old compilers 
The `__attribute__((deprecated("warning")))` macro didn't exist until
gcc 4.5 and clang 2.9.

While io.js does not build with compilers that old, add-ons do.  Let's
make src/node.h compatible with such compilers, it's a public header.

PR-URL: #1626
Refs: #1619
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
b712af7

@mscdex mscdex removed the build label May 5, 2015

@bnoordhuis bnoordhuis closed this May 5, 2015

@bnoordhuis bnoordhuis merged commit b712af7 into nodejs:master May 5, 2015

@bnoordhuis bnoordhuis deleted the bnoordhuis:fix-deprecation-macro branch May 5, 2015

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis May 5, 2015

Member

Two LGTMs should be enough, right? Landed with style adjustments in b712af7, thanks for the review.
Member

bnoordhuis commented May 5, 2015

Two LGTMs should be enough, right? Landed with style adjustments in b712af7, thanks for the review.
@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris May 5, 2015

Contributor

awesome. thanks.
Contributor

trevnorris commented May 5, 2015

awesome. thanks.

@rvagg rvagg referenced this pull request May 5, 2015

Closed

Release proposal: v2.0.1 #1629

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind May 6, 2015

Contributor

@bnoordhuis there are a few linter warnings introduced here:

src/node.h:51:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
src/node.h:59:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
Contributor

silverwind commented on b712af7 May 6, 2015

@bnoordhuis there are a few linter warnings introduced here:

src/node.h:51:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
src/node.h:59:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis May 6, 2015

Member

Well, ain't that annoying. #1640
Member

bnoordhuis replied May 6, 2015

Well, ain't that annoying. #1640

@misterdjules misterdjules referenced this pull request May 7, 2015

Open

backport NODE_DEPRECATED fixes from io.js #25267

@tojocky

This comment has been minimized.

Show comment
Hide comment
@tojocky

tojocky May 12, 2015

Why not to use message in MSC as well: "__declspec(deprecated(message)) declarator"?
Seems this is possible: https://msdn.microsoft.com/en-us/library/044swk7y.aspx

tojocky commented on src/node.h in b712af7 May 12, 2015

Why not to use message in MSC as well: "__declspec(deprecated(message)) declarator"?
Seems this is possible: https://msdn.microsoft.com/en-us/library/044swk7y.aspx

Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 19, 2015

@bnoordhuis @Fishrock123
src: fix NODE_DEPRECATED macro with old compilers 
The `__attribute__((deprecated("warning")))` macro didn't exist until
gcc 4.5 and clang 2.9.

While io.js does not build with compilers that old, add-ons do.  Let's
make src/node.h compatible with such compilers, it's a public header.

PR-URL: nodejs#1626
Refs: nodejs#1619
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
d90eadc

MylesBorins added a commit that referenced this pull request Nov 7, 2017

@MylesBorins
Working on v6.12.1 
PR-URL: #1626
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
26c4224
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment