nuttxpr commented on PR #16090: URL: https://github.com/apache/nuttx/pull/16090#issuecomment-2760691855
[**\[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 the provided information is minimal. Here's a breakdown of why and where it could be improved: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. The rationale for saving space and removing unused strings is good. * **Impact Assessment:** While terse, the impact section addresses most points, highlighting that it's an opt-in feature and should not affect existing configurations. * **Confirmation of Testing:** The PR author confirms testing, although the details are vague ("some random upstream configuration"). **Weaknesses (and suggestions for improvement):** * **Missing Issue References:** If this change relates to any existing NuttX issues, those should be linked. Even if it's a new feature not tied to a specific issue, mentioning that is helpful. * **Vague Testing:** "Some random upstream configuration" is insufficient. Specific details are crucial. List the *exact* board/config tested, the toolchain used (compiler version, etc.), and provide *meaningful* log snippets. Demonstrate that syslog calls are indeed absent in the compiled binary when the option is enabled and present when it's disabled. Size comparisons before and after the change would also be beneficial. Ideally, show output from a tool like `size` or `objdump` to quantify the space savings. * **Missing "Before" Logs:** While "after" logs are mentioned, "before" logs are equally important to demonstrate the change's effect. * **Impact on Documentation:** If this new option requires documentation updates (e.g., in the configuration system's Kconfig files or in other documentation), mention that and preferably include the updates in the PR itself. Even a simple mention in the `Impact` section like "Documentation updated in the Kconfig file for this option" is better than nothing. **In short:** The core information is there, but the PR needs more detail, particularly regarding testing and documentation, to be considered fully compliant with the NuttX requirements. Providing concrete examples and quantifiable results will significantly strengthen the PR and make it easier for reviewers to assess its correctness and value. -- 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
