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

CFP: Clang format for BPF formatting #41

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dylandreimerink
Copy link
Member

This CFP proposes to start using clang-format to format the Cilium C code, and a process for introducing it and resolving changes in formatting.

cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
cfps/CFP-041-clang-format-for-bpf-formatting.md Outdated Show resolved Hide resolved
@dylandreimerink dylandreimerink force-pushed the pr/cfp-41-clang-format-for-bpf-formatting branch 4 times, most recently from 92ea98a to a192f38 Compare July 10, 2024 13:52
@lmb
Copy link

lmb commented Jul 11, 2024

Discussed in a 1:1:

  • Add a section that describes how this affects developer workflow. In the best case people could just install clang-format locally and then the editor reformats the code on save. But that raises the question how much code changes. Experiment: format all code with the oldest version we want to support (maybe clang-format-11). Then go to the most recent version and reformat using the config from clang-11.

  • Add a snippet that we will lint out future additions of /* clang-format off */ and // clang-format off. We need to drive down the number of special cases over time, not add to them.

@dylandreimerink dylandreimerink force-pushed the pr/cfp-41-clang-format-for-bpf-formatting branch from a192f38 to 65f4efa Compare July 15, 2024 12:40
@xmulligan
Copy link
Member

@dylandreimerink we now have statuses for CFPs https://github.com/cilium/design-cfps#status. Please add the one that you think fits here

@joestringer
Copy link
Member

For context, I think this core idea was provided ample opportunity for input (for instance through sharing in Slack and community weekly meetings). The new status process only expects that prior to merge one committer approves the design that it is implementable and doesn't have major flaws / contention points. Both of you are committers so if you two agree then I would suggest marking it as "Implementable". Once implementable it can be merged, even if you still wish to iterate a bit further on the design points - those additions can come as subsequent PRs modifying this file.

@dylandreimerink dylandreimerink force-pushed the pr/cfp-41-clang-format-for-bpf-formatting branch from 65f4efa to 0a0f1d4 Compare November 13, 2024 16:05
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I'm fine with this. Do we have two volunteers to populate the committee?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants