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

Include less v2 #7570

Closed
wants to merge 2 commits into from
Closed

Include less v2 #7570

wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
Should I add one ?

Describe changes:

  • Rework includes as per cppclean

Still to do : add cppclean as part of CI

I used this patched version myint/cppclean#165

There are still exceptions :

  • About Suricata : When "rust.h" or "suricata-common.h" gets included, and when includes is rust.h or rust-context.h
  • About cppclean
    • when using macros such as TAILQ with including queue.h (or red and black tree)
    • when using the array size, such as ``uint8_t plugin_v[PLUGIN_VAR_SIZE]; in decode.h needing suricata-plugin.h

The result is less includes from header file to another, nothing is done for C files including a useless header

Modifies #7563 with compilation fixed for Victor's options and adding one commit removing 3 unused header files

This PR adds about one hundred lines #include "detect-engine-build.h" because it is being used in util tests for SigGroupBuild and such... Any thoughts on that ?

@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #7570 (791e15c) into master (a898409) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7570      +/-   ##
==========================================
- Coverage   75.80%   75.73%   -0.07%     
==========================================
  Files         658      658              
  Lines      186526   186525       -1     
==========================================
- Hits       141399   141273     -126     
- Misses      45127    45252     +125     
Flag Coverage Δ
fuzzcorpus 59.83% <ø> (-0.13%) ⬇️
suricata-verify 52.35% <ø> (-0.07%) ⬇️
unittests 60.72% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on ips_afp_drop_chk.

Pipeline 7997

@catenacyber catenacyber added the needs rebase Needs rebase to master label Jul 6, 2022
@catenacyber
Copy link
Contributor Author

@jasonish I would like your thoughts before rebasing (as it will likely be painful)

@jasonish
Copy link
Member

jasonish commented Jul 7, 2022

I do like where this is going: removals from .h files, and more added to the .c files makes sense.

The Rust headers are super includes by nature, not much we can do about that I don't think.

Is there much manual work involved? Or is it just run the tool?

@catenacyber
Copy link
Contributor Author

Is there much manual work involved? Or is it just run the tool?

The tool just spots useless includes in header files.
Then, you have to manually put this include in the C files needing it...

@catenacyber catenacyber mentioned this pull request Jul 7, 2022
@catenacyber
Copy link
Contributor Author

Continues in #7618

@catenacyber catenacyber closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

3 participants