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

Mutex BasicLockable #1441

Merged
merged 2 commits into from
May 10, 2022
Merged

Conversation

connorfuhrman
Copy link
Contributor

Originating Project/Creator Connor Fuhrman
Affected Component None
Affected Architectures(s) None
Related Issue(s) #1229
Has Unit Tests (y/n) y
Builds Without Errors (y/n) y
Unit Tests Pass (y/n) y
Documentation Included (y/n) y

Change Description

Added unlock alias for Os::Mutex type.

Rationale

Os::Mutex is now BasicLoackable.

Testing/Review Recommendations

Added test in Os folder but is really only a compile-time check. Perhaps there is a better way to assert at compile-time that the BaiscLockable requirements are satisfied. For now a std::lock_guard is instantiated with an Os::Mutex parameter to show correctness.

Future Work

When/if F' C++ standard bumps to C++17 or above Mutex could be upgraded to satisfy the Lockable requirement.

@connorfuhrman connorfuhrman changed the title Enh/mutex basiclockable Mutex BasicLockable May 10, 2022
@LeStarch
Copy link
Collaborator

@timcanham is this going to affect you? I think the C++11 feature is limited to the UTs and projects who use this feature. Thoughts?

@timcanham
Copy link
Collaborator

@LeStarch I don't see that it could. It's just another public method, and we won't try to run those unit tests on the target.

@LeStarch
Copy link
Collaborator

Thanks!

@LeStarch LeStarch merged commit c8672e2 into nasa:devel May 10, 2022
LeStarch pushed a commit that referenced this pull request Jun 29, 2022
* Added mutex unlock alias

* Added test that Mutex is 'BasicLockable'
@connorfuhrman connorfuhrman deleted the enh/mutex_basiclockable branch September 26, 2022 20:38
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.

3 participants