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

[core] Add a shared mutex class #2981

Merged
merged 11 commits into from
Aug 1, 2024
Merged

Conversation

yomnes0
Copy link
Collaborator

@yomnes0 yomnes0 commented Jul 11, 2024

Add a shared mutex class

@yomnes0 yomnes0 marked this pull request as ready for review July 12, 2024 06:36
@maxsharabayko maxsharabayko added Type: Enhancement Indicates new feature requests [core] Area: Changes in SRT library core labels Jul 12, 2024
@maxsharabayko maxsharabayko added this to the v1.5.4 milestone Jul 12, 2024
@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Jul 12, 2024

@yomnes0

  • Please review the style and indention formatting used in this PR. Those should follow the project style.
  • Please add unit tests for the class.


Mutex m_Mutex;
int m_iCountRead;
bool m_bWriterLocked;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two fields can be combined into one field. Please see /usr/include/c++/v1/shared_mutex.

Would be also nice that you use exactly the same public API as the standard one so that a prospective drop-in replacement with the standard one from C++17 can be done later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about merging those two fields. I however felt that it would hurt readibility for a negligible memory and performance gain.

API has been changed to match the standard one

Improved syntax
Changed lock/unlock functions name to match reference implementation
Add unit tests for SharedMutex
@yomnes0 yomnes0 marked this pull request as draft July 15, 2024 13:03
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Show resolved Hide resolved
srtcore/sync.cpp Show resolved Hide resolved
@yomnes0 yomnes0 marked this pull request as ready for review July 29, 2024 12:43
@maxsharabayko
Copy link
Collaborator

@yomnes0
The PR's status has been changed to "Ready for review", but only formatting comments have been addressed.

srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
srtcore/sync.cpp Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
srtcore/sync.h Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko merged commit ffdebd3 into Haivision:master Aug 1, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants