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

src: use CHECK(false) in switch default case #26502

Closed
wants to merge 6 commits into from
Closed

src: use CHECK(false) in switch default case #26502

wants to merge 6 commits into from

Conversation

nitsakh
Copy link
Contributor

@nitsakh nitsakh commented Mar 7, 2019 •
edited
Loading

Passing a !string to CHECK will trigger a string conversion warning and it's also inconsistent with the usage in other places. Hence, this changes it to CHECK(false) making it consistent.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 7, 2019
bnoordhuis
bnoordhuis reviewed Mar 8, 2019
View reviewed changes
src/node_crypto.cc Outdated Show resolved Hide resolved
nitsakh added a commit to electron/node that referenced this pull request Mar 8, 2019
@nitsakh
src: use CHECK(false) in switch default case
97b5f65 
Porting nodejs/node#26502
@nitsakh
Copy link
Contributor Author

nitsakh commented Mar 8, 2019

Thanks! I just updated the branch as per @bnoordhuis 's suggestion. Please let me know if this approach works.

@nitsakh
Copy link
Contributor Author

nitsakh commented Mar 8, 2019 •
edited
Loading

Maybe the build is failing on gcc4.9 due to https://github.com/nodejs/node/blob/master/src/node_file.h#L151-L156 ? 🤔

@BridgeAR BridgeAR requested review from bnoordhuis and danbev March 10, 2019 20:50
bnoordhuis
bnoordhuis reviewed Mar 11, 2019
View reviewed changes
src/util.h Outdated Show resolved Hide resolved
@danbev
Copy link
Contributor

danbev commented Mar 14, 2019

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

@nitsakh
Copy link
Contributor Author

nitsakh commented Mar 14, 2019 •
edited
Loading

UNREACHABLE is called in a template definition (here), and static cannot be used inside a constexpr function. Hence, I changed the static const to constexpr for definition of node::AssertionInfo args in the ERROR_AND_ABORT macro (it's better to indicate constexpr to the compiler anyway).
This, I think, is causing the failure on gcc4.9, because we are redefining constexpr for gcc4.9 here.
I'm opting to change the declaration to const, I think that'll fine™.

@danbev
Copy link
Contributor

danbev commented Mar 15, 2019

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

bnoordhuis
bnoordhuis previously requested changes Mar 15, 2019
View reviewed changes
src/util.h Show resolved Hide resolved
src/util.h Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Mar 19, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/21652/
Resume: https://ci.nodejs.org/job/node-test-pull-request/21665/

@nitsakh
Copy link
Contributor Author

nitsakh commented Mar 20, 2019

@bnoordhuis, it'll be great if you can please take a quick look at the changes.

codebytere pushed a commit to electron/node that referenced this pull request Mar 20, 2019
@nitsakh @codebytere
src: use CHECK(false) in switch default case
67596d8 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 22, 2019
@nitsakh
src: use CHECK(false) in switch default case
c886c42 
Porting nodejs/node#26502
@BridgeAR BridgeAR requested a review from bnoordhuis March 25, 2019 17:52
nitsakh added a commit to electron/node that referenced this pull request Mar 25, 2019
@nitsakh
src: use CHECK(false) in switch default case
45d26ec 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 25, 2019
@nitsakh
src: use CHECK(false) in switch default case
05fc9f9 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 26, 2019
@nitsakh
src: use CHECK(false) in switch default case
4bb65f1 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 26, 2019
@nitsakh
src: use CHECK(false) in switch default case
9999e67 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 26, 2019
@nitsakh
src: use CHECK(false) in switch default case
a99125c 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 26, 2019
@nitsakh
src: use CHECK(false) in switch default case
d46fb53 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Mar 26, 2019
@nitsakh
src: use CHECK(false) in switch default case
3050493 
Porting nodejs/node#26502
nitsakh added a commit to electron/node that referenced this pull request Apr 11, 2019
@nitsakh
src: use CHECK(false) in switch default case
dc1ea71 
Porting nodejs/node#26502
zcbenz pushed a commit to electron/node that referenced this pull request Apr 16, 2019
@nitsakh @zcbenz
src: use CHECK(false) in switch default case
6e15b0c 
Porting nodejs/node#26502
zcbenz pushed a commit to electron/node that referenced this pull request Apr 16, 2019
@nitsakh @zcbenz
src: use CHECK(false) in switch default case
5e4a13a 
Porting nodejs/node#26502
@nitsakh
Copy link
Contributor Author

nitsakh commented Apr 17, 2019

Please let me know if there's something blocking here. Happy to make needed changes. :-)

@addaleax addaleax dismissed bnoordhuis’s stale review April 17, 2019 17:49

has been addressed

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. and removed crypto Issues and PRs related to the crypto subsystem. labels Apr 17, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ryzokuken
Copy link
Contributor

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

1 similar comment
@nodejs-github-bot
Copy link
Collaborator

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

@nitsakh nitsakh force-pushed the check-no-string branch from 5db2562 to 6d4b624 Compare May 15, 2019 18:14
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

Sorry this took so long! Landed in 00ba75e 🎉

@addaleax addaleax closed this May 19, 2019
addaleax pushed a commit that referenced this pull request May 19, 2019
@nitsakh @addaleax
src: update UNREACHABLE macro to take a string
00ba75e 
PR-URL: #26502
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request May 20, 2019
@nitsakh @targos
src: update UNREACHABLE macro to take a string
d21e066 
PR-URL: #26502
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
codebytere pushed a commit to electron/node that referenced this pull request Jun 20, 2019
@nitsakh @codebytere
src: use CHECK(false) in switch default case
c8b62e7 
Porting nodejs/node#26502
@codebytere codebytere mentioned this pull request Oct 2, 2019
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@sam-github sam-github sam-github left review comments

@danbev danbev danbev approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs 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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nitsakh @danbev @refack @BridgeAR @nodejs-github-bot @ryzokuken @addaleax @sam-github @bnoordhuis