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: fix useless call in permission.cc #46833

Merged
merged 1 commit into from
Feb 27, 2023
Merged

src: fix useless call in permission.cc #46833

merged 1 commit into from
Feb 27, 2023

Conversation

tniessen
Copy link
Member

FromMaybe() has no side effects and the return value is ignored. Use Check() instead to ensure that the operation succeeded.

@tniessen tniessen added c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. labels Feb 25, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 25, 2023
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
addaleax
addaleax reviewed Feb 25, 2023
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I agree that the current state here isn't ideal, but I'd rather fix it by doing proper error handling (i.e. return; on empty Maybe) than using .Check() and adding potential crashes here

@tniessen
Copy link
Member Author

@addaleax I went with Check() because the same statements already use ToLocalChecked(). Would you prefer to replace only Check() or both functions with proper error handling?

@addaleax
Copy link
Member

addaleax commented Feb 25, 2023 •
edited
Loading

@tniessen I think in ideal world we'd also handle errors from the string creation functions, but since they can only fail (afaik) if the strings exceed the maximum length of strings allowed by V8, and they (currently) only accept input that is already defined at compile time, so I don't have strong feelings about those

(The extra std::string() call here is another story, but... 🤷‍♀️)

@tniessen
Copy link
Member Author

@addaleax Thank you, that makes sense. I'll update it soon.

@tniessen
src: fix useless call in permission.cc
0e95c5a 
FromMaybe() has no side effects and the return value is ignored.
Instead, if Set() fails, then another exception is pending, so
return early.
@tniessen tniessen force-pushed the src-permission-throwaccessdenied-useless-call branch from 5989869 to 0e95c5a Compare February 25, 2023 21:37
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2023
@github-actions github-actions bot mentioned this pull request Feb 27, 2023
Open
13 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit 3abbc38 into nodejs:main Feb 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 3abbc38

@github-actions github-actions bot mentioned this pull request Feb 28, 2023
Open
16 tasks
targos pushed a commit that referenced this pull request Mar 13, 2023
@tniessen @targos
src: fix useless call in permission.cc
1760ae4 
FromMaybe() has no side effects and the return value is ignored.
Instead, if Set() fails, then another exception is pending, so
return early.

PR-URL: #46833
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 3, 2023
Merged
@RafaelGSS RafaelGSS added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. backport-blocked-v16.x backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. and removed dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. backport-blocked-v16.x backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@anonrig anonrig anonrig approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@apapirovski apapirovski apapirovski 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++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tniessen @nodejs-github-bot @addaleax @apapirovski @Trott @anonrig @cjihrig @RaisinTen @targos @RafaelGSS