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

.clang-format: Add default clang-format configuration #20865

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 17, 2024

Contribution description

This adds a clang-format configuration based on the Linux Kernel configuration and modified to better match RIOT's coding convention.

Testing procedure

Automatic formatting using clang-format should now match RIOT's coding convention relatively closely. Aligning bitmasks is still a pain point, though. I think this is not yet possible to configure, but something that we don't have too often. E.g. the following would still not work

/* good */
   FOO_REG = FOO_REG_VAL1
           | FOO_REG_VAL2
           | FOO_REG_VAL3;

/* bad (but sadly currently generated by clang-format) */
   FOO_REG = FOO_REG_VAL1 |
       FOO_REG_VAL2 | FOO_REG_VAL3;

Issues/PRs references

None

@maribu maribu added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels Sep 17, 2024
@github-actions github-actions bot removed the Area: tools Area: Supplementary tools label Sep 17, 2024
@maribu
Copy link
Member Author

maribu commented Sep 17, 2024

Would be interesting to know whether this would result (except for bitmasks) in correctly formatted code when using VS code.

@riot-ci
Copy link

riot-ci commented Sep 17, 2024

Murdock results

✔️ PASSED

e7c91fa .clang-format: Add default clang-format configuration

Success Failures Total Runtime
10214 0 10215 15m:14s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Nice one, thank you! I can confirm that VSCode now formats code according to our coding convention 🎉

Just some minor proposals below.

.clang-format Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
.clang-format Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
IncludeIsMainRegex: '(Test)?$'
IndentCaseLabels: false
IndentGotoLabels: false
IndentPPDirectives: AfterHash
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this also seems to include the header guard, adding an extra level of indentation in all header files :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Correction: The include guards are recognized correctly if the corresponding #endif directive is the last line of the file (and not the documentation group closing /** @} */), is that the recommended order anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is generally expected that the include guard is the first and last thing in a header. I think this will also trigger the optimisation in the preprocessor to operate faster.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I've just checked and there currently are a lot of places where the order is switched. I'll try to fix them, maybe we even get some build time improvements?

Copy link
Member Author

@maribu maribu Oct 11, 2024

Choose a reason for hiding this comment

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

There could be an improvement, but probably in the order of noise :)

You could also go for #pragma once now that there seems to be an agreement that this is an acceptable instance of using GCC and clang features that go beyond thr standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also go for #pragma once now that there seems to be an agreement that this is an acceptable instance of using GCC and clang features that go beyond thr standard.

But that would be against the coding convention (and the static tests) that explicitly ask for the include guards. So I went with #20905 now.

.clang-format Outdated Show resolved Hide resolved
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

After some manual testing with VSCode, I'd say this is ready to go. If problems showed up in the future, we could still adapt the configuration then.

@mguetschow mguetschow enabled auto-merge October 11, 2024 11:53
@mguetschow
Copy link
Contributor

@maribu squash needed :)

This adds a clang-format configuration based on the Linux Kernel
configuration and modified to better match RIOT's coding convention.

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
@mguetschow mguetschow added this pull request to the merge queue Oct 16, 2024
Merged via the queue into RIOT-OS:master with commit ed052ec Oct 16, 2024
25 checks passed
@maribu maribu deleted the clang-format branch October 16, 2024 18:50
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants