nuttxpr commented on PR #14928: URL: https://github.com/apache/nuttx/pull/14928#issuecomment-2497437778
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it: **Missing Information/Insufficient Detail:** * **Summary:** While it mentions optimization, it lacks crucial details. * **Why is this change necessary?** Is there a performance bottleneck? Are the current implementations inefficient? * **What functional part of the code is being changed?** Be specific – mention the files/functions modified (e.g., `libc/string/memcpy.c`). * **How does the change exactly work?** "Use arm32 assemble / neon instructions" is too vague. Briefly explain the optimization techniques used (e.g., loop unrolling, SIMD instructions). Mention if any new functions were added. The mention of "arch related elf parsing" seems unrelated to string functions and needs further explanation or belongs in a separate PR. * **Impact:** The impact descriptions are too brief. * **Is new feature added?** If the ELF parsing is new, say so explicitly. Otherwise, it's likely "Is existing feature changed?" is YES. * **Impact on user:** Probably NO, but if there are any changes to function behavior (however subtle), explain them. * **Impact on build:** Possibly YES, as assembly code is being added. Specify any new build flags, dependencies, or changes to the build system. * **Impact on hardware:** Specify the *specific* ARM architectures affected (armv8r, cortex-r52 is a good start, but mention if others are impacted). Are there any implications for other architectures? * **Impact on documentation:** Likely YES if new features are added or if the behavior of existing functions changes. State whether documentation updates are included in the PR. * **Impact on security:** Consider potential security implications of optimized code (e.g., buffer overflows if optimizations change memory access patterns). If none, state "NO." * **Impact on compatibility:** Are there any compatibility concerns with older versions of NuttX or other ARM architectures? * **Testing:** The testing description is inadequate. * **Build Host(s):** Provide details about the build host OS, CPU architecture, and compiler version used. * **Target(s):** Be more specific about the target board and configuration. * **Testing logs:** Absolutely essential! Include *actual* logs demonstrating the functionality before and after the change. Show performance benchmarks if performance improvement is the goal. Demonstrate correctness by showing input and output. **Example of an Improved Summary:** This PR optimizes several libc string functions (`memcpy`, `memset`, `memmove`, `memchr`, `strcmp`, `strlen`) for the armv8r architecture (specifically cortex-r52) to improve performance. The current C implementations are inefficient, particularly for larger data sizes. This optimization utilizes ARM32 assembly language and NEON SIMD instructions to perform operations on multiple bytes simultaneously. Key optimizations include loop unrolling and specialized instructions for memory operations. This PR also adds a new ELF parsing function [...] (Explain its purpose and connection, or remove this if unrelated). By providing more specific details in each section, the PR will be much clearer and easier for reviewers to evaluate. Remember to follow the contributing guidelines and provide concrete evidence of testing. -- 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]
