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

Fix some threading things and add additional thread unittests #3281

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@kwolekr
Copy link
Contributor

kwolekr commented Oct 23, 2015

  • Fix thread name reset on start()
  • Fully reset thread state on kill()
  • Add unittests to check for correct object states under various circumstances

Just making a PR for the CI checks :-)

Fix some threading things and add additional thread unittests
- Fix thread name reset on start()
- Fully reset thread state on kill()
- Add unittests to check for correct object states under various circumstances
return;
MutexAutoLock lock(m_mutex);

if (!m_joinable)

This comment has been minimized.

Copy link
@est31

est31 Oct 24, 2015

Contributor

As we have the mutex now, we don't need m_joinable anymore, right?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 24, 2015

Author Contributor

No, it still needs to be there.
A thread can't be joined more than once.

This comment has been minimized.

Copy link
@est31

est31 Oct 24, 2015

Contributor

That's why the lock is there. once the thread is joined, nobody else can obtain the lock. So a check for m_running is sufficient, no?

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Oct 24, 2015

@SmallJoker can you test whether this PR makes it compile for you?

@@ -81,9 +81,9 @@ class Thread {
/*
* Waits for thread to finish.
* Note: This does not stop a thread, you have to do this on your own.
* Returns immediately if the thread is not started.
* Returns immediately if the thread is not started or has already finished.

This comment has been minimized.

Copy link
@est31

est31 Oct 24, 2015

Contributor

Perhaps also point out what it returns?

This comment has been minimized.

Copy link
@kwolekr

kwolekr Oct 24, 2015

Author Contributor

Okay.

@kwolekr kwolekr closed this Oct 24, 2015

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.