Update documentation for the ConditionVariable API #8511
Conversation
- Hide protected member structures from doxygen. - Add some more details/comments to the example. - Add some defined/undefined behaviours. - Add some additional details to function documentation
Copy edit file, including existing text.
cmonr
added
needs: CI
release-version: 5.10.3
rollup PR
and removed
needs: review
labels
|
Note: This PR is now a part of a rollup PR (#8552). No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged. |
| @@ -90,7 +90,7 @@ struct Waiter; | |||
| * Mutex mutex; | |||
| * ConditionVariable cv(mutex); | |||
| * | |||
| * // These variables are protected by locking mutex | |||
| * // These variables are protected by locking mutex. | |||
There was a problem hiding this comment.
I guess that should be locking the mutex
| * | ||
| * // Wait for a condition to change | ||
| * cond.wait(); | ||
| * while (done == false) { |
There was a problem hiding this comment.
Good point on making this a while loop. do is pretty dangerous with a condition variable.
But == false or == true is bad style
Either done isn't a boolean, in which case it's actively dangerous, or it is a boolean, in which case it's just obfuscation.
if (done) or while (!done) on a boolean reads more like natural language.
rtos/ConditionVariable.h
Outdated
| * - The order which in which waiting threads acquire the condition variable's | ||
| * - Calling wait if the mutex is not locked by the current thread is undefined | ||
| * behavior. | ||
| * - The order in which waiting threads acquire the condition variable's | ||
| * mutex after ConditionVariable::notify_all is called is undefined. |
| * called and there are one or more waiters and one or more threads attempting | ||
| * to acquire the condition variable's mutex the order in which the mutex is | ||
| * called and there are one or more waiters, and one or more threads | ||
| * attempting to acquire the condition variable's mutex the order in which the mutex is | ||
| * acquired is undefined. |
There was a problem hiding this comment.
unspecified
Also spurious notifications below are unspecified, but don't seem to be able to put a comment there.
|
@melwee01 can you respond to the latest @kjbracey-arm comments in the rollup PR (it failed just now so will need to restart CI, will wait for the update before I restart, investigating why it compiled some code that should not). |
ghost
deleted the
Description
Pull request type