nuttxpr commented on PR #15213: URL: https://github.com/apache/nuttx/pull/15213#issuecomment-2547099751
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown: **Missing/Insufficient Information:** * **Summary:** * While it mentions using atomic operations and fixing a regression, it doesn't explain *what* `f_refs` is, *why* it needs atomic protection, and *how* the atomic operations achieve this protection. What was the specific issue caused by the regression? * It mentions including another PR (#15202). This should be clarified – is this PR dependent on #15202? Does it supersede #15202? Simply stating "includes" is ambiguous. * **Impact:** * "RELEASE" is not descriptive enough. Does this mean it affects a release branch? Which one? The Impact section requires "YES/NO" answers for each category followed by descriptions if "YES." At minimum, it needs to explicitly address each of the impact categories (user, build, hardware, documentation, security, compatibility). Even if the answer is NO, it should state "NO." * **Testing:** * "CI" is also insufficient. While Continuous Integration testing is valuable, the requirements specifically ask for details about *local* testing. What host and target platforms were tested? What specific tests were run? The provided log sections are empty. Real testing logs are needed to demonstrate the fix and that it doesn't introduce new issues. **How to Improve:** 1. **Elaborate on the Summary:** Explain what `f_refs` represents and its function. Describe the specific issue the regression introduced (e.g., race conditions, crashes, data corruption). Explain how using atomic operations resolves the problem. Clarify the relationship with PR #15202. 2. **Complete the Impact Assessment:** Address *all* impact categories with "YES/NO" and provide explanations for any "YES" responses. If there is no impact in a category, explicitly state "NO." For "RELEASE," specify which release is affected. 3. **Provide Detailed Testing Information:** List the specific build host(s) and target(s) used for testing, including OS, CPU architecture, compiler, and board/configuration details. Include actual testing logs (or relevant snippets) demonstrating the problem before the change and the correct behavior after the change. Even if CI tests pass, demonstrating local testing is essential for review. By providing this missing information, the PR will meet the NuttX requirements and be easier for reviewers to understand and evaluate. -- 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]
