Skip to content

Cleanup includes#160

Merged
natoscott merged 2 commits intohtop-dev:masterfrom
BenBE:cleanup-includes
Oct 19, 2020
Merged

Cleanup includes#160
natoscott merged 2 commits intohtop-dev:masterfrom
BenBE:cleanup-includes

Conversation

@BenBE
Copy link
Copy Markdown
Member

@BenBE BenBE commented Sep 19, 2020

This updates all source files to mention exactly those includes required for compiling.

Tested with IWYU 0.12 + clang 9. Please report issues with other setups.

NB: IWYU sometimes has issues with finding certain declarations and reports compile issues in these cases. If plain clang compiles then this is an issue with IWYU. Such issues can usually be ignored.

FWIW: IWYU sees functions used inside defines as being used where the macro is used, not where the define is declared. This is a good reason to adopt #154 to avoid this issue.

As I'm sure there will be compile issues on some platforms with this as is I'm just drafting this for now.

@BenBE BenBE force-pushed the cleanup-includes branch 5 times, most recently from 869ced0 to 55d6e09 Compare September 19, 2020 13:31
This was referenced Sep 19, 2020
@BenBE BenBE marked this pull request as ready for review September 23, 2020 16:40
@ghost
Copy link
Copy Markdown

ghost commented Sep 23, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.318 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@cgzones cgzones added the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 5, 2020
@BenBE BenBE force-pushed the cleanup-includes branch 2 times, most recently from ba919d1 to 4ee33ca Compare October 7, 2020 20:41
@BenBE
Copy link
Copy Markdown
Member Author

BenBE commented Oct 7, 2020

The order of includes I established in this PR is:

  1. "config.h" with a IWYU pragma to always keep it (required for conditional defines and certain system macros)
  2. header for the file being compiled
  3. all system headers in alphabetical order, sub directories at the end
  4. all our headers in alphabetical order, sub directories at the end
  5. all conditionally included headers, i.e. checked by #if defined(…)

AFTER the block of includes two blank lines should be present before anything else.

For header files item 2 is simply skipped.

Important note: For functions used as part of macro defines you must add an IWYU pragma to keep these headers. Otherwise IWYU will suggest to remove them as unused AND ask each and every user of a define to include those headers. This is BTW another good reason to avoid function-like defines in headers and replace them as inline functions.

@cgzones cgzones removed the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 8, 2020
@BenBE BenBE force-pushed the cleanup-includes branch 3 times, most recently from 8d59218 to 4004cc2 Compare October 13, 2020 21:53
@cgzones cgzones mentioned this pull request Oct 14, 2020
@BenBE BenBE force-pushed the cleanup-includes branch 6 times, most recently from 15ecd94 to fc399a9 Compare October 17, 2020 08:52
Information as seen by IWYU 0.12 + clang 9 on Linux
@natoscott
Copy link
Copy Markdown
Member

The order of includes I established in this PR is:
[...]

LGTM. The 'run_iwyu.sh' script looks like it uses bash-isms - perhaps it should use a /bin/bash shebang line instead of /bin/sh? (the run_valgrind script has the same issue, so I'm gonna ignore it for now - feel free to follow up though)

[...]
This is BTW another good reason to avoid function-like defines in headers and replace them as inline functions.

Hmm - that sounds more like a bug in IWYU than a good reason to do anything, TBH.

@natoscott natoscott merged commit 0f52629 into htop-dev:master Oct 19, 2020
@BenBE BenBE deleted the cleanup-includes branch October 19, 2020 08:09
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.

3 participants