nuttxpr commented on PR #15899:
URL: https://github.com/apache/nuttx/pull/15899#issuecomment-2678304063

   [**\[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.  While it provides a 
summary of *what* the change does, it lacks crucial information:
   
   * **Missing Explanation of Necessity:**  *Why* is this change necessary? Is 
it fixing a bug?  Is it improving performance?  What was the problem before the 
change? This is the most important part of the summary.
   * **Missing Scope:**  What functional part of the code is being changed?  Be 
specific (e.g., "socket option handling", "network stack").
   * **Missing Technical Detail:** *How* does the change work? What specific 
code was modified? What are the mechanics of the fix/improvement?
   * **Missing Issue References:** Are there any related NuttX issues?  Even if 
there aren't, state that explicitly (e.g., "No related issues").
   * **Insufficient Impact Assessment:** "N/A" is rarely acceptable. Think 
carefully.  Even seemingly small changes can have unexpected impacts.  Address 
all impact categories specifically, even if the answer is "NO".  Justify your 
"NO" answers briefly (e.g., "Impact on build: NO - This change only modifies 
source code within the existing networking module and does not affect the build 
process").
   * **Missing Testing Detail:** "qemu:local" is not enough. What OS and 
compiler was qemu running on? What NuttX configuration was tested? Be specific 
about the target architecture and board configuration.
   * **Absolutely Missing Test Logs:**  Providing "before" and "after" logs is 
essential.  Show evidence that the change fixes a problem or implements a new 
feature.  Without logs, there's no way to verify the PR's claims.
   
   
   **Example of an Improved Summary:**
   
   "Fixes a potential crash in `si_getsockopt` when invalid socket options are 
passed. Previously, parameters were not checked at the top level, leading to 
possible memory corruption or other undefined behavior when attempting to 
assign invalid values.  This change adds parameter validation at the beginning 
of the function to catch these errors before any assignments occur."
   
   
   **Example of Improved Testing:**
   
   "Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
   Target: qemu-system-arm, sim:nsh
   
   Testing logs before change:
   
   ```
   nsh> some_command_that_triggers_the_bug
   Segmentation fault
   ```
   
   Testing logs after change:
   
   ```
   nsh> some_command_that_triggers_the_bug
   Invalid socket option
   ```"
   
   
   By providing this level of detail, reviewers can quickly understand the 
purpose, scope, and impact of your changes and verify that they work as 
intended.  This makes the review process much smoother and increases the 
likelihood of your PR being 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

Reply via email to