nuttxpr commented on PR #14814: URL: https://github.com/apache/nuttx/pull/14814#issuecomment-2480415968
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to **mostly** meet the NuttX requirements, but is lacking in detail and completeness. Here's a breakdown: **Strengths:** * **Clear Summary of the "What":** It clearly states *what* is being changed (removing `sched_lock` in `pthread_cond_broadcast`). * **Provides a Reason:** It explains *why* the change is believed to be necessary. * **Indicates Testing:** It mentions testing with `ci ostest`. **Weaknesses:** * **Insufficient Detail in Summary:** The *how* is missing. How exactly does removing `sched_lock` work in this context? What specific code changes were made? This is crucial for reviewers. * **Vague Impact:** "pthread cond_wait" is too general. Does this impact *all* users of condition variables? Are there performance implications (positive or negative)? Be specific. * **Missing Testing Details:** "ci ostest" isn't enough. Which specific tests within `ostest` were run? What were the *expected* outcomes? Critically, the "before" and "after" log sections are empty. Provide concrete evidence that the change works as intended. Include the actual logs. * **Unanswered Impact Questions:** The PR completely ignores most of the required "Impact" questions. This makes it difficult to assess the broader implications of the change. Each point needs a "YES" or "NO" answer, with explanation where necessary. Even if the answer is "NO," state it explicitly. * **Missing Build Host/Target Details:** Under "Testing," specify the build host operating system, compiler, target architecture, and board configuration used for testing. **Recommendation:** The PR needs significant revision before it's ready for review. Focus on providing the missing detail, particularly in the "Impact" and "Testing" sections. Provide concrete evidence of testing and address all the required questions, even if the answer is "NO." Clear, concise, and complete information is essential for efficient and effective code review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
