Skip to content

Conversation

@hujiajie
Copy link
Contributor

@hujiajiehujiajie commented Jul 25, 2017

It doesn't seem to make much sense to have the mentioned typedef
declaration equipped with NODE_EXTERN. In fact, when compiling with GCC,
an attribute specifier like __attribute__((visibility("default"))) in
such a typedef declaration will cause the following warning message:

warning: ‘visibility’ attribute ignored [-Wattributes]

The issue goes unnoticed because NODE_EXTERN is defined as nothing for
GCC builds, but for correctness it's better to not specify it here at
all.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all.
@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jul 25, 2017
Copy link
Contributor

@refackrefack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is happy

@refackrefack requested a review from trevnorrisJuly 25, 2017 08:53
@refack
Copy link
Contributor

@refack
Copy link
Contributor

/cc @nodejs/buffer @nodejs/platform-windows

refack pushed a commit that referenced this pull request Jul 29, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
@refack
Copy link
Contributor

Landed in 5796e44

@hujiajie congratulation on getting promoted by GitHub from:
image
to:
image

@refackrefack closed this Jul 29, 2017
addaleax pushed a commit that referenced this pull request Aug 1, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
@addaleaxaddaleax mentioned this pull request Aug 2, 2017
@hujiajiehujiajie deleted the ignored-attribute branch August 3, 2017 02:05
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: #14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
hferreiro pushed a commit to brave/node that referenced this pull request Nov 28, 2017
It doesn't seem to make much sense to have the mentioned typedef declaration equipped with NODE_EXTERN. In fact, when compiling with GCC, an attribute specifier like __attribute__((visibility("default"))) in such a typedef declaration will cause the following warning message: warning: ‘visibility’ attribute ignored [-Wattributes] The issue goes unnoticed because NODE_EXTERN is defined as nothing for GCC builds, but for correctness it's better to not specify it here at all. PR-URL: nodejs/node#14466 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@hujiajie@refack@trevnorris@MylesBorins@nodejs-github-bot