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

{bp-15277} include/threads.h: Replace thrd_ defines by actual function definitions #15358

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

jerpelea
Copy link
Contributor

Summary

Replace _SCHED_GETTID by pthread_self()
Update all thrd_ defines to actual function definitions

Impact

RELEASE

Testing

CI

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 27, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 27, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details.

Here's why:

  • Insufficient Summary Detail: While the summary mentions the code changes, it doesn't explain why these changes are necessary. What problem does replacing _SCHED_GETTID solve? Why update thrd_ defines? What benefit does this provide? Is this a bug fix, a performance improvement, or something else? A related NuttX issue number should be provided if applicable.

  • Missing Impact Assessment: "RELEASE" is not a sufficient impact assessment. Each impact category (user, build, hardware, documentation, security, compatibility) needs a "YES" or "NO" answer. If "YES," a description of the impact is required. For example, if this change affects backward compatibility, that needs to be explicitly stated and explained.

  • Inadequate Testing Information: "CI" is insufficient. While Continuous Integration testing is important, local testing verification is also required. The PR needs to specify the local build host (OS, CPU, compiler) and target (architecture, board, configuration) used for testing. Furthermore, "Testing logs before change" and "Testing logs after change" sections require actual logs demonstrating the issue before the change and the successful resolution after the change. Simply stating "CI" doesn't provide evidence of successful testing or demonstrate the problem being solved.

In short, the PR needs to provide more context, explain the motivations behind the changes, thoroughly assess the impact, and include concrete evidence of testing.

@xiaoxiang781216 xiaoxiang781216 merged commit abe5195 into apache:releases/12.8 Dec 28, 2024
25 of 27 checks passed
@jerpelea jerpelea deleted the bp-15277 branch December 28, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants