-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Fix(pthread): Added linux port with empty functions to make pthread cxx example build. (IDFGH-13424) (IDFGH-13433) #14339
Conversation
👋 Hello cristianfunes79, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
03b3983
to
decc2ac
Compare
301e225
to
8e2a9d2
Compare
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.
Hi @cristianfunes79, thanks a lot for this pthread Linux target implementation. Don't worry about the amount of comments, they are mostly little things! The most important question we should answer before merging, is if it should be a pure stub implementation and only return some fixed values, or if there should be a bit of logic behind the functions. Both have their advantages and disadvantages, but I think a pure stub implementation is OK for now.
Regarding the comments in the code: they are mostly little things. We could fix them ourselves in an succeeding commit. But the more cleanly we can integrate your code, the better and the more code lines with your authorship will remain in the end.
We also have pre-commit to automate some of the chore work such as adding proper license headers. Same principle here: we can fix this ourselves, but then fewer lines with your authorship remain.
Please let us know what you think about the comments. Thanks!
IMO we can keep these as stubs, this unblocks pthread cxx example to be run on linux. I can work then in another PR to add functionality to these pthread functions. What do you think? |
Sounds good! |
@cristianfunes79 One last thing: would you mind squashing the changes into one commit? |
b935ae6
to
2733099
Compare
Great, we'll integrate this ASAP. |
sha=27330993975d560971649b876b8739c005d33943 |
@cristianfunes79 We merged this PR internally. It should appear on github master soon. We had to rebase your commit due to some recent change in IDF. Your authorship will be retained, don't worry about that! However, this PR will be marked as "closed" instead of "merged". |
Great thanks, so can I pull from main now and start to work on adding functionality to this wrappers? |
This change allows to build and run cxx pthread example located in
esp-idf/examples/cxx/pthread
.