nuttxpr commented on PR #15425: URL: https://github.com/apache/nuttx/pull/15425#issuecomment-2571614402
[**\[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 missing some key information and clarity. Here's a breakdown: **Strengths:** * **Clear problem statement:** The summary explains the issue with inconsistent inline declarations and hints at a larger problem with `up_getusrsp` and exception handling. * **Impact description touches on key areas:** It mentions changes to `up_getusrsp` across architectures and claims no other side effects. * **Testing mentions CI and a specific target:** This shows some level of testing was performed. **Weaknesses:** * **Summary lacks detail on *how* the change works:** While the problem is stated, the summary doesn't explain *how* the inconsistent declarations are being fixed. What is the proposed solution? More specifics are needed. e.g., "All declarations will use `static inline` consistently" or "The redundant declaration will be removed". * **Impact is vague:** "no other side effect" is too general. Be explicit. For example: * **Impact on user:** NO * **Impact on build:** NO (assuming no build system changes are needed) * **Impact on hardware:** YES (affects all architectures due to `up_getusrsp` changes - specify which architectures were tested). * **Impact on documentation:** NO (or YES if documentation needs updating due to these changes) * **Impact on security:** NO (unless there are security implications, justify why) * **Impact on compatibility:** NO (or YES if there are compatibility concerns - be specific) * **Testing is insufficient:** "CI-test, local mps2 qemu" is not enough detail. * **Build Host(s):** Provide details about the build host OS, CPU architecture, and compiler version used. * **Target(s):** Be more specific about the target architecture and board configuration used for testing beyond "mps2 qemu." List *all* tested targets. * **Logs:** The template specifically requests "Testing logs before change" and "Testing logs after change". These are missing. Even if the logs are voluminous, a snippet demonstrating the corrected behavior is essential. **Recommendations for improvement:** 1. **Elaborate on the solution:** Explain precisely how the code is changed to address the inline declaration inconsistencies. 2. **Provide specific impact details:** Replace vague statements like "no other side effect" with explicit YES/NO answers for each impact category and provide justifications. 3. **Significantly improve the testing section:** Include details about the build host environment, all tested target configurations, and *most importantly*, provide relevant snippets of testing logs demonstrating the issue before the change and the corrected behavior after the change. If the full logs are too large, consider linking to a CI run or a separate file containing them. Even a concise summary of the test results is better than nothing. By addressing these weaknesses, the PR will better meet the NuttX requirements and be easier for reviewers to 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]
