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.

Sign up for GitHub

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

Jump to bottom

[libc] Replace MutexLock with cpp::lock_guard #89340

Merged
merged 24 commits into from
May 9, 2024
Merged

[libc] Replace MutexLock with cpp::lock_guard #89340

merged 24 commits into from
May 9, 2024

Conversation

vmishelcs
Copy link
Contributor

@vmishelcs vmishelcs commented Apr 19, 2024 •
edited
Loading

This PR address issue #89002.

Changes in this PR

  • Added a simple implementation of cpp::lock_guard (an equivalent of std::lock_guard) in libc/src/__support/CPP inspired by the libstdc++ implementation
  • Added tests for cpp::lock_guard in /libc/test/src/__support/CPP/mutex_test.cpp
  • Replaced all references to MutexLock with cpp::lock_guard

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 19, 2024 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@vmishelcs vmishelcs marked this pull request as ready for review April 19, 2024 03:01
@vmishelcs
Copy link
Contributor Author

@nickdesaulniers Please take a look whenever you find time and let me know if I need to make any changes.

Also should I leave a comment in the new mutex.h as sort of a "bibliography" that I used the GCC implementation as inspiration?

Thanks for your time and your advice earlier on how to tackle this issue!

nickdesaulniers
nickdesaulniers reviewed Apr 19, 2024
View reviewed changes
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Also should I leave a comment in the new mutex.h as sort of a "bibliography" that I used the GCC implementation as inspiration?

If you put it in the commit description in the UI on github, then it will be retained when we squash+merge. Do you mean libstdc++, or GCC? Also, it's important that you didn't copy+paste the implementation. Using it for reference is very much in the spirit of Free Software though.

Thanks for your time and your advice earlier on how to tackle this issue!

You're a pro! Good job and thank you!

libc/src/__support/CPP/mutex.h Outdated Show resolved Hide resolved
libc/src/__support/File/dir.cpp Outdated Show resolved Hide resolved
libc/src/__support/threads/fork_callbacks.cpp Outdated Show resolved Hide resolved
libc/src/__support/threads/thread.cpp Outdated Show resolved Hide resolved
libc/src/stdlib/atexit.cpp Outdated Show resolved Hide resolved
libc/src/threads/linux/CndVar.h Outdated Show resolved Hide resolved
libc/test/src/__support/CPP/mutex_test.cpp Outdated Show resolved Hide resolved
libc/src/__support/CPP/mutex.h Outdated Show resolved Hide resolved
libc/src/__support/CPP/mutex.h Outdated Show resolved Hide resolved
libc/src/__support/File/dir.cpp Outdated Show resolved Hide resolved
@vmishelcs
Copy link
Contributor Author

vmishelcs commented Apr 19, 2024 •
edited
Loading

If you put it in the commit description in the UI on github, then it will be retained when we squash+merge. Do you mean libstdc++, or GCC? Also, it's important that you didn't copy+paste the implementation. Using it for reference is very much in the spirit of Free Software though.

I used this to essentially make sure my lock_guard implementation provides the same functionality. However, no copy-pasting was done in any of my code.

@nickdesaulniers
Copy link
Member

Yeah, then it's fine to mention in the PR description that you were inspired by libstdc++'s implementation.

nickdesaulniers
nickdesaulniers reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

I'm very happy with the result here! Two minor style nits, then this LGTM.

libc/test/src/__support/CPP/mutex_test.cpp Outdated Show resolved Hide resolved
libc/src/__support/CPP/mutex.h Outdated Show resolved Hide resolved
@vmishelcs
Copy link
Contributor Author

Done and done!

@vmishelcs
Copy link
Contributor Author

@nickdesaulniers @gchatelet Friendly ping.

gchatelet
gchatelet reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@gchatelet gchatelet left a comment

Choose a reason for hiding this comment

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

Just a little nit, otherwise LGTM.
Can you land this PR or do you need me to land it?

libc/test/src/__support/CPP/mutex_test.cpp Outdated Show resolved Hide resolved
@vmishelcs
Copy link
Contributor Author

Just a little nit, otherwise LGTM. Can you land this PR or do you need me to land it?

Addressed your comment :)
I don't have write permissions so if you feel that my code meets LLVM standards I'll need someone to merge this PR.

@vmishelcs vmishelcs requested a review from gchatelet May 3, 2024 20:06
@gchatelet gchatelet merged commit c4a3d18 into llvm:main May 9, 2024
4 checks passed
@gchatelet
Copy link
Contributor

Thx for your contribution @vmishelcs ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickdesaulniers nickdesaulniers Awaiting requested review from nickdesaulniers

@gchatelet gchatelet Awaiting requested review from gchatelet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vmishelcs @nickdesaulniers @gchatelet