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

cleanup: Make all .c files include the headers they need. #2505

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 27, 2023

This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Dec 27, 2023
@iphydf iphydf marked this pull request as ready for review December 27, 2023 01:11
@iphydf iphydf requested a review from a team as a code owner December 27, 2023 01:11
@nurupo
Copy link
Member

nurupo commented Dec 27, 2023

Wow, this is great. Is there a tool for this?

We also need a commit/PR removing unnecessary includes, as this PR has added some. For example, before this PR, bootstrap_daemon/src/command_line_arguments.c replied on getting #include "log.h" from bootstrap_daemon/src/command_line_arguments.h. But now that you have added #include "log.h" directly to bootstrap_daemon/src/command_line_arguments.c in this PR, the #include "log.h" in bootstrap_daemon/src/command_line_arguments.h is no longer needed.

@nurupo
Copy link
Member

nurupo commented Dec 27, 2023

My bad, bootstrap_daemon/src/command_line_arguments.h actually needs #include "log.h". Edited my earlier comment.

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ef4897a) 69.02% compared to head (fad6e4e) 68.95%.

Files Patch % Lines
toxcore/TCP_server.c 0.00% 2 Missing ⚠️
toxcore/announce.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
- Coverage   69.02%   68.95%   -0.08%     
==========================================
  Files          89       89              
  Lines       27769    27772       +3     
==========================================
- Hits        19167    19149      -18     
- Misses       8602     8623      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iphydf
Copy link
Member Author

iphydf commented Dec 27, 2023

It was completely manually, because clang-tidy --fix makes a huge mess. I took the error messages and manually added all the includes.

@nurupo
Copy link
Member

nurupo commented Dec 27, 2023

How can I get these error messages from clang-tidy? Is there a specific check name I need to enable?

@nurupo
Copy link
Member

nurupo commented Dec 27, 2023

Looks like this might be it https://clang.llvm.org/extra/clang-include-fixer.html

@iphydf
Copy link
Member Author

iphydf commented Dec 27, 2023

It's --checks=-*,misc-include-cleaner.

Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

Reviewed 65 of 65 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 65 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @iphydf)


toxcore/ccompat.c line 6 at r1 (raw file):

#include "ccompat.h"

static_assert(sizeof(int) >= 4, "toxcore does not support 16-bit platform");

Missing an s?


toxcore/group_moderation.c line 461 at r1 (raw file):

    const int packed_len = sanctions_list_pack(packed_data, sizeof(packed_data), sanction, 1, nullptr);

    if (packed_len <= SIGNATURE_SIZE) {

JFreegman@1732318

For clarity sake, SIGNATURE_SIZE used to be a size_t, making the cast necessary. Now it's an int so this change is okay.

@toktok-releaser toktok-releaser merged commit fad6e4e into TokTok:master Dec 27, 2023
53 checks passed
@iphydf iphydf deleted the cleanup-includes branch December 27, 2023 02:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants