Skip to content

Enforce project wide #include order#911

Merged
stv0g merged 2 commits into
masterfrom
fixes-include
May 15, 2025
Merged

Enforce project wide #include order#911
stv0g merged 2 commits into
masterfrom
fixes-include

Conversation

@pjungkamp
Copy link
Copy Markdown
Contributor

@pjungkamp pjungkamp commented May 13, 2025

This instructs clang-format to enforce a uniform #include order across all files. See https://clang.llvm.org/docs/ClangFormatStyleOptions.html#includecategories

  1. ^<villas/: headers
  2. ^<[[:lower:]]+>$: standard library headers
  3. ^<.*>$: other system headers
  4. ^".*"$: local headers

@pjungkamp pjungkamp force-pushed the fixes-include branch 2 times, most recently from e4ebffa to 59c6676 Compare May 13, 2025 17:42
@pjungkamp pjungkamp marked this pull request as ready for review May 13, 2025 17:45
@n-eiling
Copy link
Copy Markdown
Member

Didn't we want to keep the clang format file as simple and as standard as possible? Is this really worth it?

@pjungkamp
Copy link
Copy Markdown
Contributor Author

I've been asked to reorder includes in multiple PRs, about half the files in this repository contain some form of error regarding our current std - other - villas header order.

I just want clang-format to sort the includes for me instead of getting a "please reorder includes" review in every second PR.

The enforced order is of cause up for debate, if you want to stick with std - other - villas.

@n-eiling
Copy link
Copy Markdown
Member

Who asks you to reorder? You should not need to do anything style related that is not mentioned here: https://villas.fein-aachen.org/docs/node/development/contributing/. This mentions nothing about include order.
From the code-perspective include order shouldn't matter?

@stv0g
Copy link
Copy Markdown
Contributor

stv0g commented May 14, 2025

I think we agreed in the past to follow the LLVM/Clang coding standards.

The minimal clang-format configuraiton which we had in the past is not sufficient, as it can not identify our local headers (e.g. #include <villas/...).

So, believe extending the clang-format config is justified as long as we stick with our goal of following the Clang/LLVM coding standard.

@pjungkamp
Copy link
Copy Markdown
Contributor Author

The LLVM #include style can be found here: https://llvm.org/docs/CodingStandards.html#include-style

I deviate from these on the "main" header of a source file. clang-format has built-in heuristics to determine a source file's "main" header. But these work best when project includes use quotes instead of angle brackets for local headers.

I've found that these heuristics determined e.g. <sys/socket.h> as the "main" include for clients/opal-rt/rtlab-asyncip/models/send_receive/src/socket.c, so I didn't enable it in this PR.

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
@stv0g stv0g merged commit 8173931 into master May 15, 2025
3 checks passed
@stv0g stv0g deleted the fixes-include branch May 15, 2025 18:58
@IgnoreWarnings
Copy link
Copy Markdown
Collaborator

IgnoreWarnings commented Jun 18, 2025

I've been asked to reorder includes in multiple PRs, about half the files in this repository contain some form of error regarding our current std - other - villas header order.

I just want clang-format to sort the includes for me instead of getting a "please reorder includes" review in every second PR.

The enforced order is of cause up for debate, if you want to stick with std - other - villas.

Great idea to automate/standardize to reduce PR review workload and frustrating comments.

@stv0g
Copy link
Copy Markdown
Contributor

stv0g commented Jun 18, 2025

We have a new pending PR by @pjungkamp to automate include ordering fixes with clang-tidy. Looking forward to this

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