Skip to content

Conversation

@nrspruit
Copy link
Contributor

No description provided.

@nrspruit nrspruit force-pushed the l0_teardown_shared_thread branch 3 times, most recently from 38b34cf to 64d2e4c Compare April 21, 2025 18:56
@nrspruit nrspruit force-pushed the l0_teardown_shared_thread branch from 2742c4f to 485c23b Compare April 21, 2025 22:33
@nrspruit nrspruit marked this pull request as ready for review April 21, 2025 22:33
@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

White space promotes readership?

std::lock_guard<std::mutex> lock(*ze_lib::stabilityMutex);
*ze_lib::stabilityPromiseResult = std::promise<int>();
*ze_lib::resultFutureResult = ze_lib::stabilityPromiseResult->get_future();
ze_lib::stabilityCheckThreadStarted->store(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight comment for readability, you are using 1 here (likely to indicate RUNNING) and back in the destruction you use -1 as a message to exit the thread. Maybe use define or enums so we can read it easier. :-)
1 = RUNNING
0 = STOPPED? QUISECED?
-1 = EXIT_THREAD?

Copy link
Contributor

@rwmcguir rwmcguir left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, my commentary can be approved tomorrow if we really don't want to change anything, since it is not functional.

@nrspruit nrspruit force-pushed the l0_teardown_shared_thread branch 2 times, most recently from f0f1c03 to 1b1146d Compare April 22, 2025 16:18
@nrspruit nrspruit force-pushed the l0_teardown_shared_thread branch from 1b1146d to c038c59 Compare April 22, 2025 16:22
@nrspruit nrspruit marked this pull request as draft April 22, 2025 16:33
@nrspruit nrspruit closed this Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants