Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp32s3_partition.c: Appease a compiler warning (-Wdiscarded-qualifiers) #14871

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 20, 2024

Summary

esp32s3_partition.c: Appease a compiler warning (-Wdiscarded-qualifiers)

Impact

no functionality changes are intended.

Testing

build tested.

@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Nov 20, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 20, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No. The PR is missing crucial information required by the NuttX guidelines. While it addresses the "what" (fixing a compiler warning), it lacks sufficient detail in almost all other areas.

Here's what's missing:

  • Summary: Needs more detail. Which compiler warning? What was the exact warning message? Which part of esp32s3_partition.c was modified? How was the warning addressed?

  • Impact: While stating "no functionality changes," it needs to explicitly answer all the impact questions with "NO" or a "YES" with a description. Even if the answer is no, explicitly stating it ensures thorough review. For example:

    • Impact on user: NO
    • Impact on build: NO (except for the removal of the warning message)
    • ...and so on for all impact categories.
  • Testing: "build tested" is insufficient. Provide details as requested:

    • Build Host(s): Specify the OS, CPU architecture, and compiler version used for testing.
    • Target(s): Specify the target architecture (e.g., sim, esp32s3). If using a simulator, which one? If hardware, specify the board and configuration.
    • Testing logs: "your testing logs here" is a placeholder. Include actual build logs (or relevant snippets) before and after the change. Showing the warning disappearing is key. If applicable, show that the functionality remains unchanged through runtime testing.

In short, the PR needs to be significantly expanded to provide the necessary context and evidence for reviewers to properly assess the change.

@xiaoxiang781216 xiaoxiang781216 merged commit 788f8fc into apache:master Nov 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture 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