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.

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

src: replace FromJust() with Check() when possible #27162

Closed

Conversation

Projects
None yet
7 participants
@sam-github
Copy link
Member

commented Apr 9, 2019

FromJust() is often used not for its return value, but for its
side-effects. In these cases, Check() exists, and is more clear as to
the intent. From its comment:

To be used, where the actual value of the Maybe is not needed like
Object::Set.

See: https://github.com/nodejs/node/pull/26929/files#r269256335


I'm not dead sure this is a good idea, because Check() doesn't exist on 11.x, but on the other hand, it's
a trivial method, we could probably backport the v8 patch for it to 11.x if it's absence was painful.

Check() is much more clear in its intent, IMO, than FromJust() called just for its side-effect.

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

@sam-github sam-github requested review from bnoordhuis and addaleax and removed request for bnoordhuis Apr 9, 2019

@sam-github sam-github force-pushed the sam-github:check-instead-of-fromjust branch from c223928 to ae8393a Apr 9, 2019

@sam-github sam-github referenced this pull request Apr 9, 2019

Draft

WIP V8 API usage in Node.js #26929

0 of 4 tasks complete
@apapirovski
Copy link
Member

left a comment

IMO a positive change.

@targos

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

00:29:00  Running C++ linter...
00:29:16  File "src/node.cc" does not use "Just"
@targos

targos approved these changes Apr 10, 2019

@bnoordhuis
Copy link
Member

left a comment

RSLGTM

sam-github added some commits Apr 9, 2019

src: replace FromJust() with Check() when possible
FromJust() is often used not for its return value, but for its
side-effects. In these cases, Check() exists, and is more clear as to
the intent. From its comment:

  To be used, where the actual value of the Maybe is not needed like
  Object::Set.

See: https://github.com/nodejs/node/pull/26929/files#r269256335

@sam-github sam-github force-pushed the sam-github:check-instead-of-fromjust branch from ae8393a to f281035 Apr 11, 2019

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@ZYSzys

ZYSzys approved these changes Apr 12, 2019

@ZYSzys ZYSzys added the author ready label Apr 12, 2019

@nodejs nodejs deleted a comment from nodejs-github-bot Apr 12, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Landed in 060d901

@sam-github sam-github closed this Apr 12, 2019

sam-github added a commit that referenced this pull request Apr 12, 2019

src: replace FromJust() with Check() when possible
FromJust() is often used not for its return value, but for its
side-effects. In these cases, Check() exists, and is more clear as to
the intent. From its comment:

  To be used, where the actual value of the Maybe is not needed, like
  Object::Set.

See: https://github.com/nodejs/node/pull/26929/files#r269256335

PR-URL: #27162
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>

@sam-github sam-github deleted the sam-github:check-instead-of-fromjust branch Apr 12, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Is it possible to make cpplint or the compiler warn (in just our src/) if the return value from FromJust() is not used? I looked at cpplint, it seems no. V8 has a macro V8_WARN_UNUSED_RESULT, perhaps some day they will decorate FromJust() with it, but it is not at the moment. Are there other options?

addaleax added a commit to addaleax/node that referenced this pull request Apr 15, 2019

worker: handle exception when creating execArgv errors
Handle possible JS exceptions that can occur by returning to JS land
immediately.

The motivation for this change is that `USE(….FromJust());` is an
anti-pattern, and `.FromJust()` with an unused return value is
superseded by `.Check()`. However, in this case, checking that the
operation succeeded is not necessary.

Refs: nodejs#27162

@addaleax addaleax referenced this pull request Apr 15, 2019

Closed

worker: handle exception when creating execArgv errors #27245

2 of 2 tasks complete

addaleax added a commit that referenced this pull request Apr 17, 2019

worker: handle exception when creating execArgv errors
Handle possible JS exceptions that can occur by returning to JS land
immediately.

The motivation for this change is that `USE(….FromJust());` is an
anti-pattern, and `.FromJust()` with an unused return value is
superseded by `.Check()`. However, in this case, checking that the
operation succeeded is not necessary.

Refs: #27162

PR-URL: #27245
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 1, 2019

V8 has a macro V8_WARN_UNUSED_RESULT, perhaps some day they will decorate FromJust() with it, but it is not at the moment.

@sam-github You could file a V8 CL that does that. Seems like a good change to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.