lupyuen commented on PR #13529: URL: https://github.com/apache/nuttx/pull/13529#issuecomment-2359708728
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13494&type=pullrequests) Let's break down this PR against the NuttX requirements: **Summary:** * **Why is change necessary?** * This part is a little weak. While it links to a prior PR and discussion, it should summarize the *reason* for those changes within this PR's description. Is it a bug fix? Performance improvement? Code clarity? * **What functional part of the code is being changed?** * Clearly identifies: `arm/cortex-a,r` architecture code, specifically related to `cp15` instructions. * **How does the change work?** * Provides a good high-level overview: * Moves `cp15.h` * Replaces instructions with macros for alignment. * Adds memory barriers. * **Related Issues/PRs:** * Links provided, which is excellent. **Impact:** * **Mostly "N/A" – This needs more detail.** Even if the impact is minimal, explain *why*. For example: * **User Impact:** "No change in user-facing behavior". * **Build Impact:** "No changes to the build process itself, though compiled output may differ slightly". * **Hardware Impact:** Specify if this is only relevant to *some* ARM/Cortex variants or if it's broad. * **Documentation:** Does `cp15.h` being moved require any documentation updates? * **Security:** Consider if using macros instead of raw instructions has any security implications (even if unlikely). * **Compatibility:** Crucial for architecture-specific changes. Are there any compatibility concerns with older ARM versions? **Testing:** * **Build Hosts:** Insufficient. List the *specific* OS, compiler versions, etc., used for testing. * **Targets:** Be specific about the ARM/Cortex architectures and boards tested on, including simulator usage if any. * **Testing logs:** While "ci-check" implies CI was used, include relevant snippets of logs showing: * **Before:** The issue/behavior the change addresses. * **After:** The successful resolution or improvement. **Overall:** The PR has a good structure and clearly explains the technical *what*. However, it's weak on the *why* (justification) and needs more thorough impact assessment and testing details. **Recommendation:** Don't just state "N/A" – explain *why* there's no impact for each category. Provide specific testing information to build confidence in the change. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
