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

   [**\[Experimental Bot, please feedback 
here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   
   
   No, this PR description does not meet the NuttX requirements. Here's why and 
how to fix it:
   
   * **Insufficient Summary:**  The summary is far too brief.  It needs to 
explain *why* a `free` command within gdb is necessary.  Is it for debugging 
memory leaks?  Is the existing `free` command in nsh insufficient in some way? 
How does the gdb command differ/improve things?  What code was changed to 
implement this (e.g., files modified)?
   
   * **Missing Impact Details:** While it notes a "New feature," the impact 
section needs to be much more detailed.  Consider these questions:
       * **Impact on user:** How will users utilize this new command? Will they 
need new build flags or configuration options?
       * **Impact on build:** Does adding this command change the build process 
at all?  Does it add any dependencies?
       * **Impact on documentation:**  This new feature *definitely* requires 
documentation updates.  Where will that documentation be added/updated?
       * **Other Impacts:**  The other impact sections (hardware, security, 
compatibility) are marked "NO" by omission. Explicitly state "NO" for each to 
be clear.
   
   * **Inadequate Testing:**  "Tested locally with a coredump" is not enough.
       * **Build Host(s):** Specify the OS, CPU architecture, and compiler 
version used for the build host.
       * **Target(s):**  Be specific about the target architecture and board 
configuration. Was this tested on a simulator (e.g., `qemu-system-arm`) or 
actual hardware?
       * **Testing logs:**  The sections for "Testing logs before change" and 
"Testing logs after change" are empty.  You *must* provide example output of 
the command before the change (presumably nothing or an error) and after the 
change (showing the memory information). Ideally, include a concise test case 
that demonstrates the command's functionality and how it addresses the problem 
it solves.
   
   
   **Example of an improved summary and testing section:**
   
   ```
   ## Summary
   
   This PR adds a `free` command to the NuttX gdb stub, similar to the nsh 
`free` command. This addresses the need for a convenient way to inspect heap 
memory usage directly within the gdb debugger, which is particularly helpful 
for identifying and diagnosing memory leaks during development. This 
enhancement allows developers to quickly assess memory allocation without 
exiting the debugging session, streamlining the debugging workflow. The changes 
primarily modify the `nuttx/debug/gdb/gdb_*.c` files to implement the command 
parsing and output formatting within the gdb stub.
   
   Related NuttX Issue: #1234 (if applicable)
   
   
   ## Impact
   
   * Is new feature added? YES
   * Impact on user (will user need to adapt to change)? YES. Users will have a 
new gdb command available. Documentation will be updated to guide usage.
   * Impact on build (will build process change)? NO
   * Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
   * Impact on documentation (is update required / provided)? YES. The gdb stub 
documentation will be updated to include the `free` command and its usage.
   * Impact on security (any sort of implications)? NO
   * Impact on compatibility (backward/forward/interoperability)? NO
   * Anything else to consider? NO
   
   
   ## Testing
   
   I confirm that changes are verified on local setup and works as intended:
   * Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
   * Target(s): sim:nsh
   
   
   Testing logs before change:
   
   ```
   (gdb) free
   Undefined command: "free".  Try "help".
   ```
   
   Testing logs after change:
   ```
   (gdb) free
   name            total      used       free       maxused    maxfree    nused 
     nfree
   GImageCache     1048576   524288     524288    786432     786432     2       
   2
   uncache         1048576   16384      1032192   32768      1032192   1        
  1
   Umem            2097152   1048576   1048576   1572864   1572864   4          
4
   (gdb)
   ```
   ```
   
   
   By providing more context, details, and evidence of testing, your PR will be 
much more likely to be accepted.  Remember to also follow the other guidelines 
in the `CONTRIBUTING.md` document.
   


-- 
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