Skip to content

Revert "toolchain/ghs: Fix CONFIG_SCHED_CRITMONITOR_MAXTIME_XXX "zero used for undefined preprocessing identifier" warnings"#13569

Merged
xiaoxiang781216 merged 1 commit intoapache:masterfrom
anchao:24092303
Sep 24, 2024
Merged

Conversation

@anchao
Copy link
Contributor

@anchao anchao commented Sep 23, 2024

Summary

Revert "toolchain/ghs: Fix CONFIG_SCHED_CRITMONITOR_MAXTIME_XXX "zero used for undefined preprocessing identifier" warnings"

move private define to public

This reverts commit 236678d.

Signed-off-by: chao an anchao@lixiang.com

Impact

N/A

Testing

ci-check

… used for undefined preprocessing identifier" warnings"

move private define to public

This reverts commit 236678d.

Signed-off-by: chao an <anchao@lixiang.com>
@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Sep 23, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 23, 2024

[Experimental Bot, please feedback here]

Let's analyze if the provided PR summary meets the NuttX requirements.

Strengths:

  • Clear Reversion: It states the PR reverts a specific commit (236678d) which is good practice.
  • Signed-off-by: Includes the contributor's sign-off.

Weaknesses and Missing Information:

  • Insufficient "Why": The summary mentions reverting a fix for warnings but doesn't adequately explain why this reversion is necessary. What problems did the original fix introduce?
  • Vague Description: "move private define to public" is too vague. What define? Why was it moved?
  • Missing Functional Context: It doesn't specify which functional area of the code is affected (e.g., scheduling, toolchain, a specific driver).
  • No Issue References: Are there related NuttX issues or pull requests? Providing links adds valuable context.
  • Incomplete Impact Assessment: "N/A" for Impact is rarely accurate. Even reversions can have consequences:
    • Compatibility: Does this reversion reintroduce compatibility issues addressed by the original commit?
    • Build: Did the original commit introduce build process changes that are now being reverted?
  • Insufficient Testing Details: "ci-check" is not enough information.
    • Specifics: What build hosts and target architectures were tested?
    • Log Snippets: Instead of full logs, provide relevant excerpts highlighting the problem before the revert and the improvement after.

Verdict:

This PR summary does not fully meet the NuttX requirements. It needs significant improvement in explaining the reasoning for the change, providing context about the affected code, and giving a thorough account of the testing performed.

@xiaoxiang781216 xiaoxiang781216 merged commit 8288fe4 into apache:master Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

4 participants