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

nitsakh · 2019-03-07T23:50:41Z

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

src/node_crypto.cc

Porting nodejs/node#26502

nitsakh · 2019-03-08T20:14:40Z

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

nitsakh · 2019-03-08T21:29:52Z

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

src/util.h

danbev · 2019-03-14T05:22:43Z

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

nitsakh · 2019-03-14T22:58:34Z

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 · 2019-03-15T05:49:15Z

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

src/util.h

refack · 2019-03-19T01:49:31Z

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

nitsakh · 2019-03-20T17:13:51Z

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

Porting nodejs/node#26502

nitsakh · 2019-04-17T17:38:10Z

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

has been addressed

nodejs-github-bot · 2019-04-17T17:50:48Z

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

nodejs-github-bot · 2019-04-17T19:40:47Z

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

nodejs-github-bot · 2019-04-29T22:14:30Z

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

nodejs-github-bot · 2019-04-30T03:40:17Z

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

ryzokuken · 2019-05-13T19:28:17Z

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

nodejs-github-bot · 2019-05-13T19:28:28Z

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

nodejs-github-bot · 2019-05-19T20:34:58Z

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

nodejs-github-bot · 2019-05-19T21:36:59Z

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

addaleax · 2019-05-19T22:07:23Z

Sorry this took so long! Landed in 00ba75e 🎉

PR-URL: #26502 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Porting nodejs/node#26502

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

nitsakh force-pushed the check-no-string branch from 4399322 to d17d1fc Compare March 8, 2019 00:36

danbev reviewed Mar 8, 2019

View reviewed changes

src/node_crypto.cc Outdated Show resolved Hide resolved

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

src: use CHECK(false) in switch default case

97b5f65

Porting nodejs/node#26502

nitsakh force-pushed the check-no-string branch from d17d1fc to 43dd472 Compare March 8, 2019 20:13

BridgeAR requested review from bnoordhuis and danbev March 10, 2019 20:50

bnoordhuis reviewed Mar 11, 2019

View reviewed changes

src/util.h Outdated Show resolved Hide resolved

danbev approved these changes Mar 14, 2019

View reviewed changes

bnoordhuis previously requested changes Mar 15, 2019

View reviewed changes

src/util.h Outdated Show resolved Hide resolved

src/util.h Outdated Show resolved Hide resolved

refack approved these changes Mar 19, 2019

View reviewed changes

codebytere pushed a commit to electron/node that referenced this pull request Mar 20, 2019

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

src: use CHECK(false) in switch default case

c886c42

Porting nodejs/node#26502

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

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

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

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

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

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

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

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

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

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

src: use CHECK(false) in switch default case

5e4a13a

Porting nodejs/node#26502

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

nitsakh added 6 commits May 15, 2019 11:11

src: update UNREACHABLE macro to take a string

a828ea3

Address review comment

7ed7094

Rename macro used

023e720

Update args

f92f828

Update based on comments

a59024b

Added space after colon in UNREACHABLE macro definition

6d4b624

nitsakh force-pushed the check-no-string branch from 5db2562 to 6d4b624 Compare May 15, 2019 18:14

addaleax closed this May 19, 2019

addaleax pushed a commit that referenced this pull request May 19, 2019

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

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 mentioned this pull request May 21, 2019

v12.3.0 proposal #27799

Merged

4 tasks

codebytere pushed a commit to electron/node that referenced this pull request Jun 20, 2019

src: use CHECK(false) in switch default case

c8b62e7

Porting nodejs/node#26502

codebytere mentioned this pull request Oct 2, 2019

chore: remove upstreamed node_crypto CHECK patch electron/electron#20406

Merged

4 tasks

Uh oh!

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

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

Uh oh!

Conversation

nitsakh commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

nitsakh commented Mar 8, 2019

Uh oh!

nitsakh commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danbev commented Mar 14, 2019

Uh oh!

nitsakh commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbev commented Mar 15, 2019

Uh oh!

Uh oh!

Uh oh!

refack commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nitsakh commented Mar 20, 2019

Uh oh!

nitsakh commented Apr 17, 2019

Uh oh!

nodejs-github-bot commented Apr 17, 2019

Uh oh!

nodejs-github-bot commented Apr 17, 2019

Uh oh!

nodejs-github-bot commented Apr 29, 2019

Uh oh!

nodejs-github-bot commented Apr 30, 2019

Uh oh!

ryzokuken commented May 13, 2019

Uh oh!

nodejs-github-bot commented May 13, 2019

Uh oh!

nodejs-github-bot commented May 19, 2019

Uh oh!

nodejs-github-bot commented May 19, 2019

Uh oh!

addaleax commented May 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nitsakh commented Mar 7, 2019 •

edited

Loading

nitsakh commented Mar 8, 2019 •

edited

Loading

nitsakh commented Mar 14, 2019 •

edited

Loading

refack commented Mar 19, 2019 •

edited

Loading