nuttxpr commented on PR #14538: URL: https://github.com/apache/nuttx/pull/14538#issuecomment-2443444655
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR summary is well-structured and informative, but it lacks a clear explanation of *why* these changes are necessary. Here's a breakdown of what's missing and how to improve it: **Summary:** * **Missing "Why":** * **cmake/gcov:** Why is it important to keep cmake consistent with the makefile in this context? Is there a problem with the current inconsistency? What benefits does consistency bring? * **gcov:** Why is it necessary to move gcov out of the sched directory? Why is it important for gcov to be a separate functional module? **Impact:** * **This section is entirely missing.** You need to address each impact point specifically. Even if there is no impact, explicitly stating "NO" is important for clarity. **Testing:** * **Insufficient Information:** * Provide the actual OS, CPU, compiler, architecture, board, and configuration details used for testing. * The testing logs sections are empty. Include relevant snippets demonstrating the issue before the change and the improvement after the change. **Example Improvements:** **Summary:** 1. **cmake/gcov:** Currently, the cmake build system handles gcov differently than the makefile system, leading to potential inconsistencies and build issues. This change aligns the cmake configuration with the makefile to ensure consistent behavior and avoid unexpected problems when building with gcov enabled. 2. **gcov:** The placement of gcov within the sched directory is illogical, as gcov is a code coverage tool unrelated to scheduling. This change relocates gcov to its own dedicated module, similar to gdbstub, for improved code organization, maintainability, and clarity. This separation also allows for more fine-grained control over gcov dependencies. **Impact:** * **Is new feature added?** NO * **Is existing feature changed?** YES (gcov configuration and build process) * **Impact on user:** YES (Users will need to adjust their build configurations to enable gcov correctly due to the dependency changes). * **Impact on build:** YES (The cmake build files are modified to handle gcov consistently with the makefile. The location of gcov source files and dependencies will change). * **Impact on hardware:** NO * **Impact on documentation:** YES (Documentation needs to be updated to reflect the new location of gcov and the modified configuration options). * **Impact on security:** NO * **Impact on compatibility:** NO (Backward compatibility is maintained) * **Anything else to consider:** NO **Testing:** * **Build Host(s):** * OS: Ubuntu 20.04 * CPU: Intel Core i7-10700K * Compiler: GCC 10.2.1 * **Target(s):** * Arch: ARM (simulated with QEMU) * Board: STM32F4 Discovery * Config: `configs/stm32f4discovery/nsh` **Testing logs before change:** ``` (Show error messages or unexpected behavior related to gcov) ``` **Testing logs after change:** ``` (Show successful gcov instrumentation and report generation) ``` By providing specific details and addressing all the required points, you make it much easier for reviewers to understand and evaluate your PR. -- 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]
