Skip to content

Conversation

@lipengfei28
Copy link
Contributor

@lipengfei28 lipengfei28 commented Oct 12, 2024

Summary

#define STRINGIFY(x)" conflicts with https://github.com/mborgerding/kissfft define

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

Franklin requirement, "#define STRINGIFY(x)" conflicts with https://github.com/mborgerding/kissfft define

Signed-off-by: chenzhijia <chenzhijia@xiaomi.com>
Signed-off-by: lipengfei28 <lipengfei28@xiaomi.com>
@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Oct 12, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 12, 2024

[Experimental Bot, please feedback here]

The provided Pull Request summary does not meet the NuttX requirements. It lacks crucial information needed for reviewers to understand and evaluate the changes.

Here's a breakdown of what's missing and how to improve it:

Summary:

  • Insufficient explanation: The summary only mentions a conflict without explaining why the change is necessary. Is this a bug fix? A compatibility improvement?
  • Missing context: What is "Franklin requirement"? Where is it defined? What are the consequences of this conflict?
  • No details on the solution: How does the PR resolve the conflict? Does it modify the STRINGIFY definition or use a different approach?

Impact:

  • All sections are empty: This section is crucial for understanding the consequences of the change. The PR author needs to analyze and describe how the change affects users, the build process, hardware, etc.

Testing:

  • No testing information provided: The PR author needs to describe how they verified the changes, including the host and target environments.
  • Missing logs: Including build and runtime logs from before and after the change is highly recommended to demonstrate the issue and the effectiveness of the fix.

To meet the NuttX requirements, the PR needs to:

  1. Provide a comprehensive summary:

    • Explain the context and why the change is needed (e.g., "The current STRINGIFY definition in [Franklin component] conflicts with the one used by kissfft library, causing build errors when both are used.").
    • Clearly describe the solution implemented (e.g., "This PR renames the conflicting STRINGIFY definition in Franklin to avoid collisions.").
    • Link to the relevant NuttX Issue if one exists.
  2. Complete the Impact section:

    • Analyze and describe the potential impact on users, build process, hardware, documentation, security, and compatibility. Be specific and provide details where applicable.
  3. Provide detailed testing information:

    • Specify the host system (OS, CPU, compiler) and target platform (architecture, board, configuration) used for testing.
    • Include build and runtime logs from before and after the change.
    • Clearly demonstrate that the change addresses the issue and does not introduce regressions.

By providing this missing information, the PR author can significantly improve the review process, making it easier for maintainers to understand, evaluate, and ultimately merge the proposed changes.

@xiaoxiang781216 xiaoxiang781216 merged commit 505adfa into apache:master Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: OS Components OS Components issues Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants