nuttxpr commented on PR #15903: URL: https://github.com/apache/nuttx/pull/15903#issuecomment-2680301390
[**\[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 crucial information. Here's a breakdown: **Strengths:** * **Clear Summary of the Problem:** The summary clearly identifies the issue (build break) and its cause (enabling `SYSLOG_RPMSG/SYSLOG_RPMSG_SERVER`). The error message provides further context. * **Signed-off-by:** Includes the `Signed-off-by` line, which is important for contribution acceptance. **Weaknesses:** * **Missing "How the Change Works":** The summary lacks a description of *how* the fix addresses the build break. What code was modified and how does it resolve the conflicting types? This is critical for reviewers to understand the change. * **Incomplete Impact Assessment:** While stating "N/A" might seem sufficient if the change is truly isolated, it's better to explicitly confirm the lack of impact in each category (user, build, hardware, documentation, security, compatibility). For example, even if there's no user impact, explicitly stating "Impact on user: NO" adds clarity. The build impact *is* affected (it was broken, now it's fixed), so that should be stated. * **Insufficient Testing Information:** The testing section is far too brief. It only states what was enabled, not the results. The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual build logs (or at least snippets showing the error before and the successful build after), and ideally also show the syslog functionality working as expected. Specify the host and target details as requested. **Recommendations for Improvement:** 1. **Explain the Fix:** Describe the code changes made to resolve the conflicting types. For example, "Modified `syslog_rpmsg_write` function signature in `syslog_rpmsg_server.c` to match the declaration in `syslog_rpmsg.h`..." 2. **Complete the Impact Assessment:** Explicitly state "NO" or provide details for *each* impact category. Specifically mention the positive impact on the build process (it now works when these options are enabled). 3. **Provide Detailed Testing Information:** * Fill in the "Build Host(s)" details: OS, CPU architecture, compiler version. * Fill in the "Target(s)" details: Architecture (e.g., simulator, real hardware), board, and configuration. * Include relevant snippets of the build logs before and after the change, demonstrating the error and the successful build. * Show evidence that syslog messages are successfully sent/received over RPMSG after the fix. By addressing these weaknesses, the PR will be much more complete and 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
