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

CODING_CONVENTIONS.md: Add IWYU policy #20570

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 14, 2024

Contribution description

This makes an implicit agreement that code should include the headers is uses, no more and no less, explicit.

It also recommends using tooling (such as clangd) and adding IWUY pragma comments to aid those tools in common cases they otherwise would provide false positives.

Testing procedure

Read the changed coding conventions and confirm that this is what we want to do.

Issues/PRs references

None

This makes an implicit agreement that code should include the headers
is uses, no more and no less, explicit.

It also recommends using tooling (such as clangd) and adding IWUY
pragma comments to aid those tools in common cases they otherwise would
provide false positives.
@maribu maribu added Area: doc Area: Documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Apr 14, 2024
@maribu maribu requested a review from jia200x as a code owner April 14, 2024 13:48
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Apr 14, 2024
@riot-ci
Copy link

riot-ci commented Apr 14, 2024

Murdock results

✔️ PASSED

e13ceeb Update CODING_CONVENTIONS.md

Success Failures Total Runtime
1 0 1 01m:00s

Artifacts

CODING_CONVENTIONS.md Outdated Show resolved Hide resolved
@chrysn
Copy link
Member

chrysn commented Apr 14, 2024

Having that done is in no way a precondition to adopting this convention, but relevant to whether we can uphold the convention: Is it realistic to have this enforced by CI? (Otherwise I fear it will be a frequently missed point in both PRs and reviews).

Co-authored-by: chrysn <chrysn@fsfe.org>
@maribu
Copy link
Member Author

maribu commented Apr 14, 2024

I think this is only relevant for hardware abstraction APIs, e.g. periph drivers and IRQ APIs. So I don't really expect new offenders to merged at a rate faster than I can fix.

I don't think that adding the pragmas can be enforcement. But having the code IWYU clean could be enforced by the CI, which would indirectly enforce suppression of false positives. But getting there will be more work than a few minutes of spare in-betweeen on a Sunday afternoon...

I might tackle that in a few months. Sounds like a sensible thing to aim for.

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.

LGTM as a coding convention, but would be good to get feedback from more experienced RIOT contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: missing approvals Integration Process: PR needs more ACKS (handled by action) Process: needs >1 ACK Integration Process: This PR requires more than one ACK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants