nuttxpr commented on PR #14955: URL: https://github.com/apache/nuttx/pull/14955#issuecomment-2500655097
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic requirements, but it lacks crucial details. While it provides a summary of the *what* and the *why*, it's missing important information related to *how* and thorough testing details. Here's what's missing and needs to be added to fully meet the requirements: * **Summary:** * **How does the change exactly work?** The summary mentions removing a mapping, but *how* is this accomplished? Code snippets or a concise explanation of the code changes would be beneficial. * **Impact:** While stating "fix broken code" is a start, it's insufficient. Specifically address *all* the impact points with NO/YES and descriptions. Even if the answer is NO, explicitly state it. For example: * Impact on user: NO * Impact on build: NO * Impact on hardware: YES (affects x86_64 architecture, potentially impacting systems relying on the `g_pt_low` mapping) * Impact on documentation: NO/YES (If documentation needs updating due to this fix, mention it here.) * Impact on security: Potentially YES (Memory access violations could have security implications. Describe this further.) * Impact on compatibility: Possibly YES (If this fixes a long-standing bug, it could affect compatibility with code that inadvertently relied on the broken behavior.) * **Testing:** The testing section is woefully inadequate. * **Build Host(s):** Provide specific details. e.g., "Linux Ubuntu 20.04, Intel i7, GCC 9.4.0" * **Target(s):** Be more specific. For QEMU: Which QEMU version? Which machine type? For Intel HW: Which specific hardware? Motherboard model? BIOS version? * **Logs:** "qemu and intel HW" isn't testing logs. Provide *actual* logs demonstrating the issue before the fix and the corrected behavior after the fix. Show how the fix resolves the problem. Ideally, include a concise test case that reliably reproduces the original issue. In short, the PR needs significantly more detail to be considered complete and ready for review. The current state provides a good starting point but lacks the necessary depth to properly evaluate the changes. -- 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]
