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

Merge threads.h into threading/thread.h #4047

Closed

Conversation

ShadowNinja
Copy link
Member

Now we only have one file for thread management.

@ShadowNinja ShadowNinja added @ Startup / Config / Util Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Apr 28, 2016
@kwolekr
Copy link
Contributor

kwolekr commented Apr 29, 2016

I'm not on board with changing all of the threadid_ts to something else.

@@ -158,9 +203,9 @@ class Thread {
Mutex m_mutex;

#if !USE_CPP11_THREADS
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be #ifndef USE_CPP11_THREADS, or (better) combined with the #if USE_CPP11_THREADS on line 220

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't change this line in this PR.

@nerzhul
Copy link
Member

nerzhul commented Apr 30, 2016

👎 for renaming threadid_t

@kwolekr
Copy link
Contributor

kwolekr commented May 26, 2016

Just a friendly ping on this PR.

I am on board with consolidating the threads code into a single file, but not the threadid_t rename. It seems unnecessary and setting that kind of precedent means we'd have to change content_t as well which is another big find & replace operation that doesn't actually provide tangible benefit to the project.

Could you split the two changes into distinct PRs? Or would you prefer that I close this one out for you and open a new one that just merges the threading files instead?

@nerzhul
Copy link
Member

nerzhul commented May 26, 2016

I have the same opinion as @kwolekr on this subject

@ShadowNinja
Copy link
Member Author

@kwolekr: Changed back to global_lowercase_underscore_t style.

@nerzhul
Copy link
Member

nerzhul commented May 19, 2017

i'm closing this, we are near feature freeze and this part will be refactored after release to use pure C++11

@nerzhul nerzhul closed this May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Startup / Config / Util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants