nuttxpr commented on PR #15217: URL: https://github.com/apache/nuttx/pull/15217#issuecomment-2547127779
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __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. This PR description **does not fully meet the NuttX requirements**. While it provides some information, it lacks crucial details and context in several areas. Here's a breakdown of the missing information: **Summary:** * **Insufficient Detail:** "Avoid spinlock/mutex by using atomic" is vague. *Which* spinlock/mutex is being replaced? *Where* in the code is this change happening? What specific function or subsystem is affected? Be more specific. For example: "The network buffer allocation routine previously used a spinlock. This PR replaces that spinlock with an atomic compare-and-exchange operation to improve performance and reduce contention." * **Missing "How":** How exactly does the atomic operation work in this context? Explain the logic flow briefly. * **Unclear Issue Reference:** Saying "includes #15104" isn't ideal. Explicitly state: "Related to NuttX Issue #15104." This makes it clear that #15104 is the related issue. **Impact:** * **RELEASE is not helpful:** Saying "RELEASE" under Impact is meaningless. Address each impact point individually (user, build, hardware, documentation, security, compatibility). Most will likely be "NO", but explain *why*. For example: "Impact on user: NO. This is an internal optimization and should not change user-facing behavior." If something *is* affected, explain clearly *how*. * **Security Implications?:** Any change involving locking mechanisms should at least consider security implications. Even if the impact is NO, explain why you believe it's not a security concern. **Testing:** * **CI is Insufficient:** "CI" tells us nothing. List the specific build hosts and targets tested. Even if it's just CI, specify which CI platforms (e.g., GitHub Actions on x86_64 Linux, simulated ARMv7-M). * **Missing Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Provide evidence that the change works as intended. This could include benchmark results, output showing the absence of race conditions, or any other relevant logs. If it's hard to demonstrate in logs, explain the testing methodology used. **In short, the PR needs much more detail to be acceptable. Focus on providing clear context, explaining the "why," "how," and "what" of the change, and backing up the claims with evidence and rationale.** -- 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]
