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.

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

Are asserts intentionally left compiled in? #14461

Closed
MSLaguana opened this issue Jul 24, 2017 · 1 comment
Closed

Are asserts intentionally left compiled in? #14461

MSLaguana opened this issue Jul 24, 2017 · 1 comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js. question Issues that look for answers.

Comments

@MSLaguana
Copy link
Contributor

  • Version: master

I was investigating perf in node-chakracore, and I noticed that there were a lot of calls to IsExternal. I was surprised to find that it was essentially due to this line: https://github.com/nodejs/node/blob/master/src/env-inl.h#L275

According to https://github.com/nodejs/node/blob/master/src/util.h#L130 ASSERT is enabled unless NDEBUG is defined and I don't see anywhere in the build that sets that. Is this intentional? There is also a CHECK macro which doesn't have the same off switch, so it does seem like asserts are intended to be disabled in release builds.
@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js. question Issues that look for answers. build Issues and PRs related to build files or the CI. labels Jul 24, 2017
@bnoordhuis
Copy link
Member

Yes, it's intentional; node has always been compiled with asserts enabled.

In fact, we should probably just get rid of ASSERT and replace it with CHECK macros. I'll do that later today if I have time.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jul 25, 2017
@bnoordhuis
src: replace ASSERT with CHECK
dc9d2fc 
Builds always have asserts enabled so there is no point distinguishing
between debug-only checks and run-time checks.  Replace calls to ASSERT
and friends with their CHECK counterparts.

Fixes: nodejs#14461
@bnoordhuis bnoordhuis mentioned this issue Jul 25, 2017
Closed
addaleax pushed a commit that referenced this issue Jul 27, 2017
@bnoordhuis @addaleax
src: replace ASSERT with CHECK
eb7faf6 
Builds always have asserts enabled so there is no point distinguishing
between debug-only checks and run-time checks.  Replace calls to ASSERT
and friends with their CHECK counterparts.

Fixes: #14461
PR-URL: #14474
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: XadillaX <admin@xcoder.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js. question Issues that look for answers.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mscdex @bnoordhuis @MSLaguana