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

Remove duplicate includes across multiple files #15474

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Jan 9, 2025

Summary

This commit cleans up redundant header file includes throughout the codebase.
The changes include:

  • Removing duplicate #include directives that were present in the same file
  • Consolidating includes that were split across multiple lines unnecessarily
  • Removing unused includes that were no longer needed
  • Fixing some formatting issues with includes

The changes improve code organization and maintainability by:

  • Reducing unnecessary dependencies
  • Making include dependencies more explicit
  • Following consistent include patterns
  • Removing dead code

No functional changes are made - this is purely a code cleanup commit.

Impact

Minor

Testing

GitHub CI

@github-actions github-actions bot added Area: Bluetooth Area: Networking Effects networking subsystem Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: renesas Issues related to the Renesas chips Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues Area: OS Components OS Components issues Area: Sensors Sensors issues Area: Audio Board: arm Board: renesas Board: risc-v Board: simulator Board: xtensa Size: M The size of the change in this PR is medium labels Jan 9, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 9, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be improved. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the what and why of the changes.
  • Addresses Impact (briefly): It acknowledges impact, though more detail would be beneficial.
  • Mentions Testing: Referencing GitHub CI is a good start.

Weaknesses:

  • Impact Lacks Detail: While stating "Minor" is concise, it doesn't fulfill the requirement to address specific impact categories (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO" for most, explicitly stating it is better.
  • Testing Lacks Detail: Relying solely on "GitHub CI" is insufficient. While CI is crucial, the PR should mention specific target architectures and configurations tested locally. Providing snippets of relevant logs (even if showing no functional change) adds further confidence.

Recommendation:

To strengthen this PR, expand the Impact and Testing sections. Example:

Impact (Improved):

  • Is new feature added? NO
  • Is existing feature changed? NO
  • Impact on user (will user need to adapt to change)? NO
  • Impact on build (will build process change)? NO (Expected to be slightly faster due to reduced parsing, but negligible)
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO
  • Impact on documentation (is update required / provided)? NO
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? NO
  • Anything else to consider? This cleanup improves code maintainability and reduces the chance of future include-related issues.

Testing (Improved):

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:nsh, arm:stm32f4discovery:nsh

Testing logs before change (sim:nsh - demonstrating no functional change):

NuttShell (NSH)
nsh> help
... (standard NSH help output) ...

Testing logs after change (sim:nsh - demonstrating no functional change):

NuttShell (NSH)
nsh> help
... (standard NSH help output) ...

By adding this level of detail, the PR becomes significantly stronger and inspires more confidence in the changes.

@github-actions github-actions bot removed the Arch: renesas Issues related to the Renesas chips label Jan 9, 2025
@no1wudi no1wudi force-pushed the clean branch 5 times, most recently from ebe4a2f to a85202e Compare January 9, 2025 08:21
 This commit cleans up redundant header file includes throughout the codebase.
 The changes include:

 - Removing duplicate #include directives that were present in the same file
 - Consolidating includes that were split across multiple lines unnecessarily
 - Removing unused includes that were no longer needed
 - Fixing some formatting issues with includes

 The changes improve code organization and maintainability by:
 - Reducing unnecessary dependencies
 - Making include dependencies more explicit
 - Following consistent include patterns
 - Removing dead code

 No functional changes are made - this is purely a code cleanup commit.

Signed-off-by: Huang Qi <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit cfd0cc0 into apache:master Jan 9, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Area: Audio Area: Bluetooth Area: Drivers Drivers issues Area: Networking Effects networking subsystem Area: OS Components OS Components issues Area: Sensors Sensors issues Board: arm Board: renesas Board: risc-v Board: simulator Board: xtensa Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants