Skip to content

Conversation

hujiajie
Copy link
Contributor

@hujiajie hujiajie 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-bot nodejs-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

@refack refack 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

@refack refack requested a review from trevnorris July 25, 2017 08:53
@refack
Copy link
Contributor

refack commented Jul 25, 2017

@refack
Copy link
Contributor

refack commented Jul 25, 2017

/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

refack commented Jul 29, 2017

Landed in 5796e44

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

@refack refack 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]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@hujiajie hujiajie 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]>
@MylesBorins MylesBorins 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues 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