nuttxpr commented on PR #14351: URL: https://github.com/apache/nuttx/pull/14351#issuecomment-2415645020
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR seems to meet most of the NuttX requirements. However, there are some areas that could be improved: **Summary:** * **Good:** Provides a clear explanation of the problem (`nuttx/arch.h` inclusion brings unnecessary headers and exposes architecture-specific details) and the proposed solution (removing the inclusion). * **Missing:** * **Functional part:** Specify the exact files and functions modified by the change. For instance, mention that the change modifies `include/nuttx/userspace.h` and removes the `#include <nuttx/arch.h>` line. * **How it works:** While you explain the "what," elaborating on the "how" would strengthen the summary. Briefly describe the mechanism used to remove the dependency on `nuttx/arch.h` without breaking user-space applications. **Impact:** * **Good:** Clearly states the impact on user-space applications and mentions no impact on other aspects. * **Needs improvement:** * **User impact:** Be more specific about how users need to adapt. Do they need to include a different header? Modify their code? Provide concrete examples if possible. * **Compatibility:** You mention no impact, but removing a header file can lead to compatibility issues. Clarify if this change guarantees backward compatibility. If not, specify the versions or scenarios affected. **Testing:** * **Good:** Mentions the tested architectures (QEMU/arm64, QEMU/x86_64). * **Missing:** * **Build Host:** Specify the operating system and compiler used for testing. * **Target (board:config):** Mention the specific boards and configurations used for testing on each architecture. * **Logs:** The template requests "testing logs before change" and "testing logs after change." While not strictly required for every PR, providing relevant log snippets demonstrating the issue before the change and the successful outcome after the change would significantly strengthen the testing section. **In summary:** The PR addresses an important issue and provides a good starting point. However, adding the missing details will make it even stronger and easier for reviewers to understand and approve. -- 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
