nuttxpr commented on PR #14063: URL: https://github.com/apache/nuttx/pull/14063#issuecomment-2405136530
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR summary is insufficient. Here's a breakdown of what's missing and how to improve it: **Summary Issues:** * **Missing "Why":** The summary states WHAT is being changed ("rename register names..."), but not WHY. Is this for consistency? To fix a bug? To support a new feature? * **Missing Scope:** What "functional part" is affected? Is it just assembly code? Specific drivers? This helps reviewers understand the potential impact area. * **Missing "How":** "Align with arm32" is vague. Provide specifics: * Are registers being renamed *to* the arm32 names, or is it a different scheme? * Are there naming conventions being followed? **Impact Issues:** * **Incomplete Sections:** Most sections are empty. You MUST address each point: * **New/Changed Features:** Even if the answer is "NO", state it explicitly. * **User Impact:** Will users see any difference? Likely not if it's internal register naming, but clarify this. * **Build Impact:** Will build systems or configurations need changes? * **Hardware Impact:** Will this affect any existing ARM64 targets? * **Documentation:** If register names appear in docs, those need updating. * **Security/Compatibility:** While unlikely to be impacted here, you must still analyze and state "NO" if applicable. **Testing Issues:** * **Missing Details:** * **Build Host:** Be specific about your OS, compiler version (e.g., GCC 12.2). * **Targets:** List ALL tested configurations: * If using simulators, which ones? QEMU versions matter. * For real hardware, provide the board name AND the NuttX configuration used (e.g., `board-name:nsh`). * **Missing Logs:** Placeholder comments aren't helpful. Provide ACTUAL logs showing the relevant behavior before and after the change. **How to Improve:** 1. **Expand the Summary:** Provide the missing WHY, SCOPE, and HOW details. 2. **Complete the Impact Assessment:** Address EACH point, even if it's to say "NO impact". Be thorough. 3. **Provide Real Testing Data:** Include build logs, console output, or anything demonstrating the change's effect (or lack thereof). **Example (Not Complete, but Illustrative):** ```markdown ## Summary This change renames ARM64 registers within the assembly code of the `arch/arm64/src/xyz` module to align with the naming conventions used by the ARM32 architecture. This is done for improved code readability and maintainability, as developers familiar with ARM32 will find the ARM64 code more intuitive. The renaming follows the ARM Architectural Reference Manual for ARMv8-A, ensuring consistency with official documentation. ## Impact * Is new feature added? NO * Is existing feature changed? NO * ... (Address ALL impact points) ## Testing I confirm that changes are verified on the following setup: * Build Host: Ubuntu 22.04, GCC 12.1 * Targets: * QEMU (version X.Y.Z): `qemu-system-aarch64 -M ...` * Raspberry Pi 4 (board config: `rpi-4:nsh`) Testing logs before change: ``` (Actual log output showing relevant register names) ``` Testing logs after change: ``` (Actual log output showing the renamed registers) ``` ``` -- 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]
