nuttxpr commented on PR #15506: URL: https://github.com/apache/nuttx/pull/15506#issuecomment-2586201730
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to **mostly** meet the NuttX requirements, but is lacking some crucial information. Here's a breakdown: **Strengths:** * **Clear Summary of Changes:** The summary clearly states the *what* and *how* of the change. Using a small lock to protect shared resources is a good practice. * **Specific File List:** Listing the impacted files is extremely helpful for reviewers. * **Mentions CI:** Indicating that Continuous Integration (CI) testing will be performed is good. **Weaknesses:** * **Missing "Why":** The summary lacks the crucial *why*. *Why* is this lock necessary? Is there a bug being fixed? Is this a performance optimization? A race condition being prevented? This is the most important part of the summary. Reference a specific NuttX issue number if one exists. * **Impact Section Incomplete:** While the file list gives a *technical* impact, the Impact section is missing the *user* impact descriptions. The PR author assumes NO impact across the board, which is unlikely. Even internal changes can have indirect effects. Think carefully about each point. For example: * **Impact on Build:** Will adding locks increase code size? Will it change any build dependencies? * **Impact on Hardware:** Will the use of locks affect real-time performance on any particular architectures? * **Impact on Security:** Does this change mitigate any potential vulnerabilities related to concurrent access to USB resources? If so, be explicit. * **Testing Section Severely Lacking:** Just saying "CI" isn't sufficient. While CI is important, *local* testing is crucial. The PR needs to demonstrate that the change actually works and fixes the intended problem (which is currently undefined). Provide specific test cases and their *before* and *after* logs. What platforms did you test on? Be explicit (e.g., "Build Host: Linux, x86_64, GCC 11.2. Target: STM32F4 Discovery board"). If it's a bug fix, show how the bug manifested before and how it's resolved after. If a performance improvement, provide benchmarks. **Recommendations for Improvement:** 1. **Explain the "Why":** Provide a concise explanation of the problem this PR solves. Is it a bug fix? Performance improvement? Preemptive fix for a potential issue? Link to a NuttX issue if one exists. 2. **Complete the Impact Section:** Address each point, even if the answer is "NO." If "NO," briefly justify why there's no impact. If "YES," provide specific details. 3. **Provide Detailed Testing Information:** Include specific test cases, platforms tested (host and target), and *before* and *after* logs demonstrating the effect of the change. Don't just rely on CI. By addressing these weaknesses, the PR will be much stronger and easier for reviewers to evaluate. A good PR saves everyone time and leads to a higher quality codebase. -- 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]
