Skip to content

node-api: avoid macro redefinition#48879

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Jul 23, 2023
Merged

node-api: avoid macro redefinition #48879

nodejs-github-bot merged 1 commit into from
Jul 23, 2023

Conversation

@tniessen
Copy link
Member

@tniessentniessen commented Jul 21, 2023

Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition.

The first definition is here:

node/src/js_native_api_v8.h

Lines 183 to 189 in b68fa59

#defineRETURN_STATUS_IF_FALSE_WITH_PREAMBLE(env, condition, status) \
do{\
if (!(condition)){\
returnnapi_set_last_error( \
(env), try_catch.HasCaught() ? napi_pending_exception : (status)); \
} \
} while (0)

The definition that is being removed is here:

node/src/js_native_api_v8.h

Lines 271 to 277 in b68fa59

#defineRETURN_STATUS_IF_FALSE_WITH_PREAMBLE(env, condition, status) \
do{\
if (!(condition)){\
returnnapi_set_last_error( \
(env), try_catch.HasCaught() ? napi_pending_exception : (status)); \
} \
} while (0)

Refs: #28237
Refs: #30006

@tniessen
node-api: avoid macro redefinition
9409def
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 21, 2023

Review requested:

  • @nodejs/node-api

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Jul 21, 2023
mhdawson
mhdawson approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessentniessen added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 21, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 21, 2023

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

vmoroz
vmoroz approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@vmorozvmoroz left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actionsgithub-actionsbot mentioned this pull request Jul 22, 2023
Open
12 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 22, 2023

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

@debadree25debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 22, 2023
@github-actionsgithub-actionsbot mentioned this pull request Jul 23, 2023
Open
12 tasks
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2023
@nodejs-github-botnodejs-github-bot merged commit 1ceb8c1 into nodejs:mainJul 23, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 23, 2023

Landed in 1ceb8c1

This was referenced Jul 24, 2023
Open
Open
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@tniessen@Ceres6
node-api: avoid macro redefinition
cb9f39a
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This was referenced Jul 28, 2023
Open
Open
Open
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
@tniessen
node-api: avoid macro redefinition
6f0085b
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
@tniessen
node-api: avoid macro redefinition
fbd169f
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@tniessen@Ceres6
node-api: avoid macro redefinition
b43a9cd
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@tniessen@Ceres6
node-api: avoid macro redefinition
6c7467c
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@tniessen@UlisesGascon
node-api: avoid macro redefinition
4c1e1ff
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs#28237 Refs: nodejs#30006 PR-URL: nodejs#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@tniessen@RafaelGSS
node-api: avoid macro redefinition
015c4f7
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: #28237 Refs: #30006 PR-URL: #48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGasconUlisesGascon mentioned this pull request Aug 15, 2023
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@tniessen@targos
node-api: avoid macro redefinition
5796631
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: #28237 Refs: #30006 PR-URL: #48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@targostargos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
node-api: avoid macro redefinition
935c497
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs/node#28237 Refs: nodejs/node#30006 PR-URL: nodejs/node#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
node-api: avoid macro redefinition
36f0e74
Even though the redefinition complies with the C standard because the second definition is "effectively the same" as the first definition, it's best to avoid any redefinition. Refs: nodejs/node#28237 Refs: nodejs/node#30006 PR-URL: nodejs/node#48879 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmorozvmorozvmoroz approved these changes

@lpincalpincalpinca approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@nodejs-github-bot@vmoroz@lpinca@mhdawson@debadree25