nuttxpr commented on PR #14921: URL: https://github.com/apache/nuttx/pull/14921#issuecomment-2496575799
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR mostly meets the NuttX requirements but needs improvements in the Impact and Testing sections. **Strengths:** * **Summary:** Clearly outlines the changes made (diagnose, thread, and memleak commands). The explanation for *why* the `thread` command change was needed is helpful. * **Testing Section (Partially):** Provides evidence of testing and shows example output. Showing the `bt` (backtrace) after the `t` (thread) command is good. **Weaknesses:** * **Impact Section:** This section is almost entirely empty. It needs to be filled out with details. Even if the answer is "NO" for some areas, it should explicitly state "NO". For example, "Impact on build: NO". Where there *is* an impact, further explanation is required. The `thread` command fix *definitely* impacts users (positively). The `memleak` optimization also impacts users. These must be described. * **Testing Section (Partially):** While example output is given, it's not clear what the *before* behavior was. It looks like the logs provided are only *after*. The testing description is also quite minimal. "Tested with qemu arm64" isn't sufficient. What configuration of qemu-arm64? Which NuttX configuration? Were other commands tested besides 'thread'? Was the `memleak` optimization tested and how? What were the before/after timings? **Specific Recommendations for Improvement:** * **Impact:** Fill out every single item. Examples: * **Impact on user:** YES. The `thread` command now functions correctly, allowing users to switch threads within GDB as expected. The `memleak` command now performs significantly faster due to caching. * **Impact on build:** NO. * **Impact on hardware:** NO. * **Impact on documentation:** YES/NO (depending on whether documentation needs updating or has been updated). If YES, describe what changes were made/are needed. * **Impact on security:** NO. (Unless there are security implications, explain if there are). * **Impact on compatibility:** Potentially YES regarding future GDB compatibility with the `thread` command change. This should be acknowledged and discussed. Perhaps a comment in the code explaining the potential future issue would also be beneficial. * **Anything else to consider:** (Mention any other impacts, edge cases, or possible future problems.) * **Testing:** * **Before Change Logs:** Add logs demonstrating the previous broken behavior of the `thread` command and (ideally) the slower performance of `memleak`. * **Target Details:** Specify the exact QEMU configuration used (e.g., `qemu-system-aarch64 -M virt -cpu cortex-a53 -nographic`). Specify the NuttX configuration (e.g., `nsh`). * **Memleak Testing:** Provide specific timings for `memleak` before and after the change to demonstrate the improvement. * **Comprehensive Testing:** Test other related GDB commands to ensure there are no unintended side effects. Consider edge cases or unusual thread states. By addressing these weaknesses, the PR will be much stronger and easier for reviewers to evaluate. It shows due diligence and helps ensure the changes are well-considered and thoroughly tested. -- 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
