Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

worker: add missing initialization #41421

Closed
wants to merge 1 commit into from
Closed

worker: add missing initialization #41421

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 6, 2022

Add missing initialization reported by coverity

Signed-off-by: Michael Dawson mdawson@devrus.com

@mhdawson
worker: add missing initialization
1a46893 
Add missing initialization reported by coverity

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Jan 6, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jan 8, 2022
Open
20 tasks
addaleax
addaleax requested changes Jan 8, 2022
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

uv_thread_t is an opaque type, we should not rely on the fact that 0 may currently be a valid initailizer for it.

Usage of tid_ is restricted to the condition that thread_joined_ is false, and conversely thread_joined_ is only set to false when tid_ is initialized. Consequently, this seems to be a false positive.

Edit: To be clear: This also reduces readability because initializing tid_ makes it looks like the variable can always be accessed, even when the thread is not running, which is not valid usage of it. However, I think we’re using C++17 now, so maybe we could drop thread_joined_ and instead make tid_ an std::optional<uv_thread_t>?

@github-actions github-actions bot mentioned this pull request Jan 9, 2022
Open
18 tasks
addaleax added a commit to addaleax/node that referenced this pull request Jan 9, 2022
@addaleax
src: use std::optional for Worker thread id
2f968dc 
Refs: nodejs#41421
addaleax added a commit to addaleax/node that referenced this pull request Jan 9, 2022
@addaleax
src: use std::optional for Worker thread id
f177794 
Refs: nodejs#41421
@addaleax addaleax mentioned this pull request Jan 9, 2022
Merged
@addaleax
Copy link
Member

addaleax commented Jan 9, 2022

PR with that suggestion: #41453

@mhdawson
Copy link
Member Author

Ok, I'll check the coverity report after the change proposed in #41453 and if it still reports an issue we can flag it as a false positive.

This was referenced Jan 12, 2022
Open
Open
Open
aduh95 pushed a commit that referenced this pull request Jan 14, 2022
@addaleax
src: use std::optional for Worker thread id
c22177f 
Refs: #41421

PR-URL: #41453
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jan 16, 2022
@addaleax @targos
src: use std::optional for Worker thread id
ec9071f 
Refs: #41421

PR-URL: #41453
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
@addaleax @mawaregetsuka
src: use std::optional for Worker thread id
a2aaaed 
Refs: nodejs#41421

PR-URL: nodejs#41453
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@mhdawson
Copy link
Member Author

#41453 seems to have resolve the warning, closing.

@mhdawson mhdawson closed this Jan 17, 2022
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
@addaleax @thedull
src: use std::optional for Worker thread id
58e840e 
Refs: nodejs#41421

PR-URL: nodejs#41453
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@addaleax @Linkgoron
src: use std::optional for Worker thread id
c102d01 
Refs: nodejs#41421

PR-URL: nodejs#41453
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@addaleax addaleax addaleax requested changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mhdawson @nodejs-github-bot @addaleax @cjihrig @tniessen