nuttxpr commented on PR #15325: URL: https://github.com/apache/nuttx/pull/15325#issuecomment-2560841254
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements. While it provides *some* information, it lacks crucial details and context. Here's a breakdown: **Missing/Insufficient Information:** * **Summary:** The summary is extremely brief and doesn't adequately explain the *why* behind the change. Simply stating that `g_current_regs` is only used to determine if we're in an IRQ and that other functionalities are removed isn't enough. What were the other functionalities? *Why* were they removed? What problem did this solve or what improvement did it create? What are the benefits of this change? * **Impact:** The impact section is almost entirely empty. While it says "no" seemingly in reference to adding a new feature, it needs to explicitly address *all* the points: impact on user, build, hardware, documentation, security, compatibility. Even if the answer is "NO" for each, it should be stated explicitly. If there *is* an impact, it needs to be described. * **Testing:** The testing section is severely lacking. It only mentions "ostest". This doesn't provide any useful information. What is "ostest"? What are the test results? It's crucial to provide: * **Build Host Details:** Operating System (e.g., Linux Ubuntu 20.04), CPU architecture (e.g., x86_64), Compiler (e.g., GCC 10.2.0). * **Target Details:** Architecture (e.g., ARM Cortex-M4), Board and Configuration (e.g., STM32F4 Discovery). * **Logs:** "Before" and "After" logs demonstrating the change's effect. These logs should be relevant to the functionality being modified and show that the intended behavior is achieved. **Recommendations for Improvement:** 1. **Expand the Summary:** Explain the rationale behind the change. Justify the removal of functionalities. Highlight the benefits (e.g., reduced code size, improved performance, bug fix). Provide context. 2. **Complete the Impact Section:** Explicitly address *all* impact points, even if the answer is "NO". If there *is* an impact, describe it in detail. 3. **Provide Detailed Testing Information:** Specify the build host and target environments completely. Include relevant "Before" and "After" logs that demonstrate the change's effect and verify that it works as intended. 4. **Consider adding a section on how to reproduce the bug if this is a bug fix.** **Example of an Improved Testing Section:** ``` ## Testing I confirm that changes are verified on my local setup and work as intended: * **Build Host:** Linux Ubuntu 22.04, x86_64, GCC 11.2.0 * **Target:** ARM Cortex-M4, STM32F4 Discovery, `nsh` configuration **Testing logs before change:** ``` nsh> some_command # Command that uses the removed functionality Segmentation fault ``` **Testing logs after change:** ``` nsh> some_command ... (expected output showing the correct behavior without the removed functionality) ... nsh> irqinfo # showing the irq state is still accessible ... (output showing interrupt information) ... ``` ``` By providing more detail and following the NuttX PR requirements, the review process will be much smoother, and the changes will be more likely to be accepted. -- 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
