-
-
Notifications
You must be signed in to change notification settings - Fork 216
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
Implement lock-free fast_random function #596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR! Here are a few comments.
src/internal_modules/roc_core/target_posix/roc_core/fast_random.cpp
Outdated
Show resolved
Hide resolved
src/internal_modules/roc_core/target_posix/roc_core/fast_random.cpp
Outdated
Show resolved
Hide resolved
src/internal_modules/roc_core/target_posix/roc_core/fast_random.cpp
Outdated
Show resolved
Hide resolved
3345b23
to
250f7ec
Compare
250f7ec
to
f41392e
Compare
Not sure if it's ready for review, but I checked new commits, the new version looks good! I think this CI job failed because you have different version of clang-format. https://github.com/roc-streaming/roc-toolkit/actions/runs/6436815832/job/17480913363?pr=596 Which one do you use? I'm using 14.0.6. I think you can rollback changes in hashmap.h and check-formatting job will pass. Other CI jobs failed because fast_random() is missing doxygen comment, please add it. Please ping me when CI is fixed and PR is ready for review. |
Weirdly enough formatting is still somehow an issue and i also have clang-format 14.0.6 |
I've pushed commit to this PR's branch that rolls back formatting: 671b26d Here is how you can run the same version of clang-format that is used on CI:
it should produce the same results that are expected on CI. Ideally we need to investigate what's the problem and maybe adjust hashmap code to be unaffected by it, but we have ongoing issue that also touches hashmap (#579) and I'd like to avoid creating conflicts. So for now, if you will work on something else in roc, you can use the docker command above. We can return to this problem later if needed. Thanks for the patch! I'll merge it after CI passes. |
Commit for #426