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: Elaborate on C11 compliance #21140

Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 19, 2025

Contribution description

This adds reasoning for requiring standard compliant C code, and lists generally accepted exceptions to this rule.

Testing procedure

Read the changes and reasoning.

Issues/PRs references

https://forum.riot-os.org/t/notes-virtual-maintainer-assembly-vma-2024-11/4428

@maribu maribu requested a review from jia200x as a code owner January 19, 2025 21:17
@github-actions github-actions bot added the Area: doc Area: Documentation label Jan 19, 2025
Comment on lines +73 to +74
- `__attribute__((used))`, `__attribute__((section("...")))`,
`__attribute__((weak))`, and `__attribute__((alias("...")))`
Copy link
Member Author

Choose a reason for hiding this comment

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

Notably missing but used in the code base:

  • __attribute__((always_inline)): This should be IMO a wrapper in compiler_hints.h. A compiler not supporting this will generate less efficient code (assuming this is actually sensibly used), but correct code, if this is ignored
  • __attribute__((align(N)))/attribute((aligned(N))): There is now <stdalign.h>that providesalignas()and the_Alignas()` since C11, so no need to use an extension for this.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I think this is useful information to have. You get 1 ACK from me :)

@maribu maribu added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Jan 20, 2025
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Jan 20, 2025
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.

Just two nits, otherwise another ACK from my side. Thanks for writing this up!

CODING_CONVENTIONS.md Outdated Show resolved Hide resolved
CODING_CONVENTIONS.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Jan 20, 2025
This adds reasoning for requiring standard compliant C code, and lists
generally accepted exceptions to this rule.

Co-authored-by: mguetschow <[email protected]>
@maribu maribu force-pushed the CODING_CONVENTIONS.md/std-c-exceptions branch from b437929 to efa622b Compare January 20, 2025 10:55
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jan 20, 2025
@riot-ci
Copy link

riot-ci commented Jan 20, 2025

Murdock results

✔️ PASSED

efa622b CODING_CONVENTIONS.md: Elaborate on C11 compliance

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

Artifacts

@MrKevinWeiss MrKevinWeiss added this pull request to the merge queue Jan 20, 2025
Merged via the queue into RIOT-OS:master with commit ea2818d Jan 21, 2025
27 checks passed
@maribu maribu deleted the CODING_CONVENTIONS.md/std-c-exceptions branch January 21, 2025 09:20
@maribu
Copy link
Member Author

maribu commented Jan 21, 2025

Thx :)

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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs 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.

5 participants