nuttxpr commented on PR #15705: URL: https://github.com/apache/nuttx/pull/15705#issuecomment-2614794492
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although it could be improved. Here's a breakdown of why and suggestions for improvement: **Strengths:** * **Clear Summary:** The summary explains the "why" (semantic consistency with Linux), the "what" (renaming and adding spinlock functions), and the "how" (new functions disable preemption). * **Code Examples:** Including the code changes as examples is helpful. * **Signed-off-by:** Indicates contributor agreement to the Developer Certificate of Origin. **Weaknesses & Suggestions:** * **Impact Understated:** Marking impact as "N/A" is likely incorrect. Even seemingly minor changes to core functions like spinlocks can have significant impacts. Consider these: * **Impact on user:** While the user *might* not need to change code directly, this changes the semantics of existing functions. This could lead to subtle bugs if users were relying on the old behavior. At a minimum, the PR should explain why user impact is *believed* to be minimal. * **Impact on build:** Are any Kconfig options affected? Do any existing users need to change their configurations? * **Impact on hardware:** Does this change affect any specific architectures? If it's generic scheduling code, probably not, but it's worth explicitly stating that. * **Impact on documentation:** The documentation definitely needs updating to reflect the new functions and renamed functions. The PR should mention this and ideally include the documentation updates. * **Impact on compatibility:** Does this break any existing code that used `raw_spin_*`? If so, how will users migrate their code? If not, explicitly state backward compatibility is maintained. * **Testing Insufficient:** "ci-check" is not sufficient testing information. The requirements ask for specific build hosts, targets, and *logs*. Provide details on the platforms tested and example logs demonstrating the change's correct behavior. Ideally, include a test case that specifically exercises the new preemption-disabling spinlocks. **In short:** While the core information is present, this PR needs more detail in the Impact and Testing sections to fully meet the NuttX requirements. Be explicit about potential impacts even if you believe them to be minimal, and provide concrete testing evidence. -- 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
