nuttxpr commented on PR #15391: URL: https://github.com/apache/nuttx/pull/15391#issuecomment-2566068294
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description, while providing some information, needs significant improvement to meet the NuttX requirements fully. Here's a breakdown and suggestions: **Weaknesses:** * **Summary is insufficient:** While it explains the *why*, it lacks crucial details about *what* was changed and *how*. It mentions `tcb->sigdeliver` and a new flag `TCB_FLAG_SIGDELIVER`, but doesn't specify the code modifications. Imagine someone reviewing this without seeing the code diff; they wouldn't understand the change. * **"Impact: None" is too brief:** Even seemingly small changes can have impacts. This needs more thorough consideration. At minimum, justify *why* there's no impact. Was the removed code truly unused? * **Testing is inadequate:** Saying "ostest is executed without failure" isn't enough. Provide specific commands used, expected output, and actual output. "smp environment of armv7a and armv8a" is also vague; specify the boards and configurations used. Crucially, the "Testing logs before change" and "Testing logs after change" sections are empty. * **No Issue references:** Even if there isn't a directly related issue, consider whether this addresses a broader problem that might have been discussed previously. * **Drawback section misplaced and insufficient:** This belongs in the "Impact" section. Also, "There are no drawback so far" is weak. More robust testing and reasoning are needed to confidently assert this. **Suggested Improvements:** **Summary:** * **What functional part is changed?** Specifically name the files and functions modified. (e.g., "The signal delivery mechanism in `sched/signal/sig_deliver.c` and the TCB structure in `sched/include/nuttx/sched.h` are modified.") * **How does the change exactly work?** Provide a concise, technical description. (e.g., "The `tcb->sigdeliver` pointer, previously used to indicate pending signal delivery, is removed. A new flag, `TCB_FLAG_SIGDELIVER`, in the `tcb->flags` field now serves this purpose. Calls to assign and check `tcb->sigdeliver` are replaced with setting/checking this flag. This reduces memory usage and simplifies the logic.") **Impact:** * **Impact on user:** Even if there's no direct impact, state it explicitly and justify it. (e.g., "NO. This is an internal optimization and does not affect user-facing APIs or behavior.") * **Impact on build:** Likely NO, but confirm. * **Impact on hardware:** Specify the architectures and boards tested. Justify why other architectures are *not* affected, if applicable. * **Impact on documentation:** If no documentation changes are needed, state it explicitly. (e.g., "NO. This change is internal and does not require documentation updates.") * **Impact on security:** Analyze potential security implications. Even if none are apparent, explicitly state your reasoning. (e.g., "NO. This change simplifies the signal delivery logic, potentially reducing the attack surface, but introduces no new vulnerabilities.") * **Impact on compatibility:** Address backward compatibility. Does this change affect applications compiled against older NuttX versions? * **Anything else to consider?** Mention any performance improvements observed. **Testing:** * **Build Host(s):** Provide full details (e.g., "Linux Ubuntu 20.04, x86_64, GCC 9.4.0") * **Target(s):** Be specific (e.g., "ARMv7-A: STM32F429I-DISC1, `nsh` configuration; ARMv8-A: QEMU virt, `nsh` configuration") * **Testing logs:** Include relevant logs demonstrating the behavior before and after the change. Focus on the signal delivery functionality. Show how the new flag is being used. Even simple logs demonstrating successful signal handling are better than nothing. Use commands like `dmesg` or application-specific logging. If using `ostest`, provide specific test case outputs. By addressing these points, you'll create a much stronger PR that's easier to review and more likely to be accepted. Remember to also follow the NuttX Contributing Guidelines closely. -- 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
