nuttxpr commented on PR #14058: URL: https://github.com/apache/nuttx/pull/14058#issuecomment-2405023608
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) **No, this PR does not fully meet the NuttX Requirements.** Here's why and how to improve it: **Summary:** * **Missing "Why":** You haven't explained the reasoning behind these changes. * **Why** modify the return type of `up_perf_gettime`? What problem does it solve, or what improvement does it offer? * **Why** add critical section protection to the clock/perf code? Is there a specific concurrency issue this addresses? * **Lack of Detail:** * **"modify the return value..."** Be more specific. What was the old return type? What is `clock_t` and why is it more suitable? * **"add critical section protection"** Where exactly is this protection added? What specific race conditions were possible before? **Impact:** * **Unlikely to be "none":** * **Return Type Change:** Changing the return type of a function almost always impacts users, as existing code calling that function may need modification. * **Critical Section:** Adding synchronization primitives can have performance implications. **Testing:** * **Insufficient:** "sim" is not descriptive enough. Which simulator? Which architecture? * **Missing Logs:** You need to provide actual log output from your tests, both before and after the changes. This helps reviewers understand the problem you're solving and verify your fix. **How to Improve the PR:** 1. **Expand the Summary:** Provide a clear and concise explanation of the "why" behind your changes. Be specific about the issues being addressed. 2. **Detailed Impact Analysis:** Carefully consider all potential impacts, even if they seem minor. For example: * **Compatibility:** Will existing applications break due to the return type change? If so, is there a migration path? * **Performance:** Will the critical section introduce noticeable overhead in performance-critical code paths? 3. **Thorough Testing:** * Specify the exact simulator(s) and architectures used for testing. * Include relevant log snippets demonstrating the problem before the change and the correct behavior after the change. **Remember:** A good PR focuses on clarity and providing reviewers with all the information they need to understand, evaluate, and merge your changes effectively. -- 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]
