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

stdio.h:Remove unnecessary header file, app header change #2832

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

anjiahao1
Copy link
Contributor

Summary

Remove stdio.h unnecessary header file

Impact

Many compilation configs rely on removing redundant header files in stdio

Testing

build sim:nsh

@nuttxpr
Copy link

nuttxpr commented Nov 8, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it addresses some aspects, it lacks crucial details. Here's a breakdown of what's missing:

Summary:

  • Insufficient explanation: "Remove stdio.h unnecessary header file" is too vague. Which files are having stdio.h removed? Why is it unnecessary in those specific instances? What problem does this solve (e.g., circular dependencies, reduced code size, improved compile times)?
  • Missing context: Is this related to a specific issue in NuttX or NuttX Apps? If so, provide the links.

Impact:

  • Vague Impact Description: "Many compilation configs rely on removing redundant header files in stdio" is unclear. How do they rely on this? Does this fix a build error? Does it improve performance? Be specific.
  • Incomplete Impact Assessment: The PR description only addresses build impact. It needs to address all other impact categories (user, hardware, documentation, security, compatibility) with either "NO" or "YES (with explanation)". For example:
    • Impact on user: NO
    • Impact on build: YES (Reduces dependencies and potentially improves compile times for certain configurations)
    • Impact on hardware: NO
    • Impact on documentation: NO (or YES if documentation needs updating)
    • Impact on security: NO
    • Impact on compatibility: NO (or YES with explanation if there are compatibility concerns)

Testing:

  • Insufficient Testing Details: "build sim:nsh" is not enough. What specific commands were run in the NSH console before and after the change? What was the expected output in both cases? Provide actual logs or snippets demonstrating the problem before the change and the successful outcome after the change.
  • Missing Build Host Information: Specify the build host OS, CPU architecture, and compiler version used for testing.

Example of a better PR Description:

Summary

This PR removes unnecessary inclusion of stdio.h from the following files: file1.c, file2.c, and file3.c. These files were including stdio.h despite not using any functions or types defined in it. This redundant inclusion increased compilation time and code size, particularly in minimal configurations. This change addresses issue #[NuttX Issue Number (if applicable)].

Impact

  • Impact on user: NO
  • Impact on build: YES (Reduces dependencies and improves compile times, particularly for configurations that exclude standard C library features). Build size may be slightly reduced.
  • Impact on hardware: NO
  • Impact on documentation: NO
  • Impact on security: NO
  • Impact on compatibility: NO

Testing

Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0

Target: sim:nsh

Testing logs before change:

nsh> date  // Took 0.5 seconds to execute (example)
... other commands and their slow execution times ...

Testing logs after change:

nsh> date // Took 0.1 seconds to execute (example)
... other commands showing improvement or at least no regression ...

This improved example provides much more context and detail, making it easier for reviewers to understand and evaluate the PR. Remember to adapt it to your specific changes and provide concrete evidence of the problem and the solution.

@hartmannathan
Copy link
Contributor

Hello,

The PR description is a bit confusing: It says remove stdio.h, but the PR does not remove stdio.h. It adds other includes. Perhaps a better description is "Add missing includes" or something like that?

Thanks

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a commit message to document the new headers

@anjiahao1
Copy link
Contributor Author

@hartmannathan apache/nuttx#14697 instdio.hremove <nuttx/xxxx.h> , Some .c previously depended on <nuttx/xxx.h> and needed to be added to .c instead of stdio.h

stdio.h remove <nuttx/xxxx.h>  Some .c previously depended on
<nuttx/xxx.h> and needed to be added to .c instead of stdio.h

Signed-off-by: anjiahao <anjiahao@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit dfbf408 into apache:master Nov 12, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants