nuttxpr commented on PR #15349: URL: https://github.com/apache/nuttx/pull/15349#issuecomment-2562639207
[**\[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 in Summary:** * **Why is the change necessary?** You state the conflict, but not *why* resolving it this way is the best approach. Is there another way to handle this conflict? Why is making it a weak function preferred? Explain the rationale. * **What functional part of the code is being changed?** Be specific. Mention the file(s) modified. For example: "The `atomic.h` header and associated architecture-specific implementation files." * **How does the change *exactly* work?** Explain what "making it a weak function" means in practical terms. What happens when both `libc` and `libcxx` define `atomic_fetch_x` now? Which one takes precedence under different configurations? **Missing Information in Impact:** * **Is new feature added? Is existing feature changed?** This modifies existing functionality, so specify that. * You mention `CONFIG_LIBC_ARCH_ATOMIC`, but you haven't explained the impact. Expand on these points: * **Impact on user:** Will users notice any changes in behavior? Will they need to change their code? If `libcxx` provides the atomic operations, what happens if a user was relying on the NuttX version? * **Impact on build:** Does this change require any modifications to build scripts or Kconfig options? * **Impact on hardware:** Likely NO, but state it explicitly. * **Impact on documentation:** If this changes user-facing behavior, documentation likely needs updating. State whether you've updated it (YES) or not (NO, and explain why not if applicable). * **Impact on security:** Are there any potential security implications of this change? Even if NO, explicitly state it. * **Impact on compatibility:** This change directly addresses a compatibility issue, so explain *how* it improves compatibility. Is backward compatibility maintained? * **Anything else to consider?** This section is for unusual edge cases or potential future implications. If none, state "None." **Missing Information in Testing:** * **Build Host(s):** Provide details. e.g., "Linux, x86_64, GCC 12.2" * **Target(s):** Be more specific. "esp32c3 board:default config" * **Testing logs before change:** Include actual logs demonstrating the conflict. Show the error or undesired behavior. * **Testing logs after change:** Show logs demonstrating that the issue is resolved. Show the expected behavior now that the function is weak. Demonstrate both scenarios: one where `libcxx` provides the function and one where it doesn't (if applicable). **Example of an Improved PR Description:** ``` ## Summary This change addresses a symbol conflict between NuttX's `atomic_fetch_x` implementation (enabled by `CONFIG_LIBC_ARCH_ATOMIC`) and the same interface provided by `libcxx`. This conflict results in build errors when both are used. To resolve this, the NuttX `atomic_fetch_x` functions are now declared as weak symbols. This allows `libcxx`'s implementation to take precedence if it exists, while still providing the NuttX version as a fallback if `libcxx` is not used. The files modified include `include/atomic.h` and the architecture-specific atomic operation implementation files (e.g., `arch/risc-v/src/common/atomic.c`). ## Impact * Is new feature added? NO * Is existing feature changed? YES, the `atomic_fetch_x` functions are now weak symbols. * Impact on user: Users who rely on `libcxx`'s atomic operations will now transparently use them. Users who do *not* use `libcxx` but have `CONFIG_LIBC_ARCH_ATOMIC` enabled will continue to use the NuttX implementations. No code changes are required. * Impact on build: NO * Impact on hardware: NO * Impact on documentation: YES. The documentation for `CONFIG_LIBC_ARCH_ATOMIC` has been updated to reflect this change and explain the interaction with `libcxx`. (Provide link to documentation changes or commit message showing updates). * Impact on security: NO * Impact on compatibility: This change improves compatibility with projects that use `libcxx`. Backward compatibility is maintained for projects that don't use `libcxx`. Applications using the NuttX atomic functions without `libcxx` will continue to function as before. * Anything else to consider: None. ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, x86_64, GCC 12.2 * Target(s): esp32c3 board:default config Testing logs before change: ``` /path/to/build/file.cpp:123: error: multiple definition of 'atomic_fetch_add_explicit' ... other linker errors ... ``` Testing logs after change (with libcxx): ``` // Successful build, output demonstrating libcxx's atomic_fetch_add_explicit is used. ``` Testing logs after change (without libcxx): ``` // Successful build, output demonstrating NuttX's atomic_fetch_add_explicit is used. ``` ``` By providing more detail and context, you'll make it much easier for reviewers to understand and approve your PR. Remember to tailor this example to your specific changes. -- 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]
