Skip to content

Formatting for C/C++/CUDA files - #1107

Open
manauref wants to merge 9 commits into
agent_toolsfrom
agent_tools-clang_format
Open

Formatting for C/C++/CUDA files#1107
manauref wants to merge 9 commits into
agent_toolsfrom
agent_tools-clang_format

Conversation

@manauref

@manauref manauref commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Here we use clang-format to:

  1. Standardize the format of all C/C++/CUDA files (except for those in minus/ and ker/).
  2. Add agent and CI tools so these formats are automatically preserved.

This is a different take on (but influenced by) PR #939 .

Standard format

We adopt a Linux kernel style (even though this isn't the industry standard for C++/CUDA, but we like it). We made two modifications to the style via the .clang-format config file:

  • Use 2-space indentation.
  • Pack function arguments on a line (rather than each argument in a separate line).

If you have modifications to the standard you'd like to see, please voice it in the comments below. Note: we tweak a change here or there, maybe, but we should avoid over-tweaking because it's best if we stay as close as possible to an international standard (in this case the Linux kernel style). And keep in mind that no one (myself included) will be happy with every aspect of the style, but this will be the last time we have to talk about it.

Formatting CI

One may apply the clang-format formatter manually, see bottom of README. But we add two things so that we rarely have to do this:

  • A .pre-commit-config.yaml file so that pre-commit automatically formats things when you do git commit, see README.
  • We add a .github/workflows/format-check.yml file so that GitHub CI checks for the correct format.
  • An agent SKILL file so agents format things correctly automatically.

One ask:
Git blame will see the commit that reformatted a given line, blaming me. If you want to see the actual code change, you'll have to git blame twice. I'm fine with this, as:

  1. we rarely use git blame
  2. if you suspect this PR broke something, git blame will point you to this PR/commit.
    That said, if you prefer that I remove these commits/this PR from git blame, let me know, I think I can.

@manauref
Add files needed for clang-format, including the agent SKILL so agent…
dfbb073 
…s follow the format.

Entire-Checkpoint: 01M1PJFZ062E1B910297FAR1QA
@manauref
Apply clang-format formatting across core/moments/vlasov/gk/pkpm, exc…
259f8da 
…luding minus/ and ker/ directories (and Lua files).

Entire-Checkpoint: 01M1PJV0HET3EMX0N5VJHFWPM2
@manauref
Move clang-format stuff to README.
fd98442
@manauref
Place format-all.sh in ci/.
a540642 
Entire-Checkpoint: 01M1PMKG3FG6WFPAV25Z5M1EKA
@manauref
manauref requested review from Antoinehoff, JunoRavin, Maxwell-Rosen, jjz73 and tnbernard and removed request for Maxwell-Rosen September 4, 2026 16:54
@manauref manauref mentioned this pull request Sep 4, 2026
Closed
4 tasks
@manauref
Merge remote-tracking branch 'origin/agent_tools' into agent_tools-cl…
ddce78f 
…ang_format

# Conflicts:
#	core/unit/ctest_alloc.c
#	core/unit/ctest_array.c
#	core/unit/ctest_array_average.c
#	core/unit/ctest_array_dg_reduce.c
#	core/unit/ctest_array_integrate.c
#	core/unit/ctest_array_ops.c
#	core/unit/ctest_array_reduce.c
#	core/unit/ctest_basis.c
#	core/unit/ctest_block_geom.c
#	core/unit/ctest_block_topo.c
#	core/unit/ctest_dg_array_mask.c
#	core/unit/ctest_dg_basis_ops.c
#	core/unit/ctest_dg_bin_ops.c
#	core/unit/ctest_dg_differentiate.c
#	core/unit/ctest_dual_num.c
#	core/unit/ctest_dynvec.c
#	core/unit/ctest_eval_offset_fd.c
#	core/unit/ctest_eval_on_nodes.c
#	core/unit/ctest_fv_proj.c
#	core/unit/ctest_gauss_quad.c
#	core/unit/ctest_linsolvers.c
#	core/unit/ctest_mat.c
#	core/unit/ctest_mat_triples.c
#	core/unit/ctest_math.c
#	core/unit/ctest_mpack.c
#	core/unit/ctest_multib_comm_conn.c
#	core/unit/ctest_null_comm.c
#	core/unit/ctest_proj_on_basis.c
#	core/unit/ctest_proj_powsqrt_on_basis.c
#	core/unit/ctest_range.c
#	core/unit/ctest_rect_decomp.c
#	core/unit/ctest_rect_grid.c
#	core/unit/ctest_ref_count.c
#	core/unit/ctest_rrobin_decomp.c
#	core/unit/ctest_tensor_field.c
#	core/unit/ctest_tensor_field_ops.c
#	core/unit/mctest_mpi_comm.c
#	core/unit/mctest_mpi_comm_read.c
#	core/unit/mctest_nccl_comm.c
#	gyrokinetic/unit/ctest_ambi_bolt_potential.c
#	gyrokinetic/unit/ctest_asdex.c
#	gyrokinetic/unit/ctest_block_tensor.c
#	gyrokinetic/unit/ctest_correct_maxwellian_gyrokinetic.c
#	gyrokinetic/unit/ctest_deflate_zsurf.c
#	gyrokinetic/unit/ctest_deflated_dg_bin_ops.c
#	gyrokinetic/unit/ctest_deflated_fem_poisson.c
#	gyrokinetic/unit/ctest_dg_cx.c
#	gyrokinetic/unit/ctest_dg_gyrokinetic.c
#	gyrokinetic/unit/ctest_dg_gyrokinetic_kern_tm.c
#	gyrokinetic/unit/ctest_dg_interpolate.c
#	gyrokinetic/unit/ctest_dg_iz.c
#	gyrokinetic/unit/ctest_dg_rad_gyrokinetic.c
#	gyrokinetic/unit/ctest_dg_recomb.c
#	gyrokinetic/unit/ctest_efit.c
#	gyrokinetic/unit/ctest_fem_parproj.c
#	gyrokinetic/unit/ctest_fem_poisson_perp.c
#	gyrokinetic/unit/ctest_fem_poisson_perp_ksq.c
#	gyrokinetic/unit/ctest_gk_geometry_mapc2p.c
#	gyrokinetic/unit/ctest_gk_geometry_mirror.c
#	gyrokinetic/unit/ctest_gk_geometry_tok.c
#	gyrokinetic/unit/ctest_gkgeom.c
#	gyrokinetic/unit/ctest_gkneut_hamil.c
#	gyrokinetic/unit/ctest_gyrokinetic_cross_prim_moms_bgk.c
#	gyrokinetic/unit/ctest_gyrokinetic_pol_density.c
#	gyrokinetic/unit/ctest_integrated_moms.c
#	gyrokinetic/unit/ctest_loss_cone_mask_gyrokinetic.c
#	gyrokinetic/unit/ctest_ltx_miller.c
#	gyrokinetic/unit/ctest_mirror_grid_gen.c
#	gyrokinetic/unit/ctest_mom_gyrokinetic.c
#	gyrokinetic/unit/ctest_nodal_ops.c
#	gyrokinetic/unit/ctest_position_map.c
#	gyrokinetic/unit/ctest_positivity_shift_gyrokinetic.c
#	gyrokinetic/unit/ctest_proj_gk_bimaxwellian_on_basis.c
#	gyrokinetic/unit/ctest_proj_gk_maxwellian_on_basis.c
#	gyrokinetic/unit/ctest_time_roots.c
#	gyrokinetic/unit/ctest_translate_dim.c
#	gyrokinetic/unit/mctest_multib_allgather.c
#	gyrokinetic/zero/gk_collisionless_flux.c
#	gyrokinetic/zero/gk_collisionless_flux_cu.cu
#	gyrokinetic/zero/gk_collisionless_passive_flux.c
#	gyrokinetic/zero/gk_collisionless_passive_flux_cu.cu
#	moments/unit/ctest_fem_helmholtz.c
#	moments/unit/ctest_fem_poisson.c
#	moments/unit/ctest_fem_poisson_vareps.c
#	moments/unit/ctest_gr_spacetime.c
#	moments/unit/ctest_ten_moment_nn_closure.c
#	moments/unit/ctest_wave_geom.c
#	moments/unit/ctest_wave_geom_helpers.c
#	moments/unit/ctest_wv_apply_bc.c
#	moments/unit/ctest_wv_euler.c
#	moments/unit/ctest_wv_euler_mixture.c
#	moments/unit/ctest_wv_euler_rgfm.c
#	moments/unit/ctest_wv_gr_euler.c
#	moments/unit/ctest_wv_gr_euler_tetrad.c
#	moments/unit/ctest_wv_gr_maxwell.c
#	moments/unit/ctest_wv_gr_maxwell_tetrad.c
#	moments/unit/ctest_wv_gr_medium.c
#	moments/unit/ctest_wv_gr_mhd.c
#	moments/unit/ctest_wv_gr_mhd_tetrad.c
#	moments/unit/ctest_wv_gr_twofluid.c
#	moments/unit/ctest_wv_gr_twofluid_tetrad.c
#	moments/unit/ctest_wv_gr_ultra_rel_euler.c
#	moments/unit/ctest_wv_gr_ultra_rel_euler_tetrad.c
#	moments/unit/ctest_wv_iso_euler.c
#	moments/unit/ctest_wv_iso_euler_mixture.c
#	moments/unit/ctest_wv_maxwell.c
#	moments/unit/ctest_wv_mhd.c
#	moments/unit/ctest_wv_reactive_euler.c
#	moments/unit/ctest_wv_sr_euler.c
#	moments/unit/ctest_wv_ten_moment.c
#	moments/unit/ctest_wv_vacuum_einstein.c
#	moments/unit/ctest_wv_vacuum_einstein_conformal.c
#	vlasov/unit/ctest_bc_basic.c
#	vlasov/unit/ctest_canonical_pb_continuity.c
#	vlasov/unit/ctest_canonical_pb_equilibrium.c
#	vlasov/unit/ctest_correct_maxwellian.c
#	vlasov/unit/ctest_correct_mj_integrated.c
#	vlasov/unit/ctest_dg_em_vars.c
#	vlasov/unit/ctest_dg_lbo_vlasov.c
#	vlasov/unit/ctest_dg_maxwell.c
#	vlasov/unit/ctest_dg_vlasov.c
#	vlasov/unit/ctest_hyper3x_dg.c
#	vlasov/unit/ctest_hyper_dg.c
#	vlasov/unit/ctest_mom_vlasov.c
#	vlasov/unit/ctest_positivity_shift_vlasov.c
#	vlasov/unit/ctest_prim_vlasov.c
#	vlasov/unit/ctest_proj_mj_on_basis.c
#	vlasov/unit/ctest_spitzer_coll_freq.c
Antoinehoff
Antoinehoff reviewed Sep 4, 2026
View reviewed changes
Comment thread core/apps/lw_priv.c Outdated
Antoinehoff
Antoinehoff reviewed Sep 4, 2026
View reviewed changes
Comment thread core/creg/rt_kann_bench.c Outdated
Antoinehoff
Antoinehoff reviewed Sep 4, 2026
View reviewed changes
Comment thread core/creg/rt_kann_cmp_arch.c
Antoinehoff
Antoinehoff reviewed Sep 4, 2026
View reviewed changes
Comment thread gyrokinetic/creg/rt_gk_tcv_iwl_adapt_source_3x2v_p1.c Outdated
@Maxwell-Rosen

Maxwell-Rosen commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

I love this. A formatting scheme is very valuable. We can nitpick the details, but that's not a valuable discussion. We should get it to a form that it's minimally abrasive and just apply it everywhere. Everyone has their own style of what coding should look like, but it's not a fruitful discussion. It's more important that the codebase is uniform rather than tuned for every individual's preferences

@manauref
Summary of changes (uncommitted, ready for your review)
8897853 
New tool: ci/strip-trailing-commas.py — a dependency-free Python script that
removes any comma immediately before a closing } in initializer lists, since
clang-format explodes a braced list onto one line per element whenever a trailing
comma is present. It's comment/string/raw-string-literal-aware (won't touch commas
inside those) and supports --check for CI-style verification.

Wired in everywhere clang-format already runs, always before clang-format so the
comma is gone by the time it formats:
- ci/format-all.sh — runs the stripper then clang-format, in both normal and
  --check modes.
- .pre-commit-config.yaml — new local hook (strip-trailing-commas) ahead of the
  existing clang-format hook.
- CI needs no changes — .github/workflows/format-check.yml already runs
  pre-commit/action, which auto-picks up the new hook.
- README.md and .agents/skills/code_style/SKILL.md — documented the new behavior.

Style change: .clang-format — added Cpp11BracedListStyle: false, so braced
initializers get spaces inside ({ 1, 2, 3 } instead of {1, 2, 3}), independent of
the trailing-comma fix.

One-time repo-wide sweep: ran both tools across the whole repo — 970 tracked files
reformatted (existing trailing commas stripped, then re-run through clang-format
with the new brace spacing).

Verified: unit-level regex edge cases, full --check clean, pre-commit run
--all-files clean, the installed git hook actually strips a reintroduced trailing
comma and blocks the commit until re-staged, and a full make -j3 rebuild succeeds
with no compilation errors.

Not yet done (per the plan): committing this as one dedicated sweep commit and
appending its SHA to .git-blame-ignore-revs, so git blame continues to skip
mechanical reformatting commits — ready whenever you want to commit.

Entire-Checkpoint: 01M1Q79P818MQCPG1YC75CAYDP
@manauref
Switch .clang-format file to use the exact Linux kernel .clang-format…
2956aec 
… file from Linus Torvald's GitHub, with the only change being: A) no tabs, B) 2-space indents, C) 100-character column width.

Entire-Checkpoint: 01M1QC9AR7ERWVX18E2Y8FMS31
@manauref

manauref commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Ok, after looking at some files and looking into @Antoinehoff's feedback, I came to the conclusion that, like @Maxwell-Rosen said, nit-picking almost anything is not particularly productive. I tried to improve some of the things Antoine pointed out, and wasn't able to without "breaking"* other things. Formatters just have their limitations if you are at the same time trying to adopt some or all of an accepted standard (in this case Linux kernel style).

So i've opted to:

  1. Remove trailing commas in struct assignments because some users put them, others don't, and this triggers different behavior in the formatter.*
  2. Use the latest Linux kernel style (.clang-format file from Linus Torvald's github), with 3 mods:
  • 100 character line limit.
  • No tab characters to indent.
  • Indents are 2 blank space.

we'll all be equally unhappy, but won't have to deal with style this ever again.

*I also tried adding trailing commas in struct initializers. This is much harder, but I did it. However the problem is that it breaks apart every struct initialization into multiple lines, which we don't want.

@Antoinehoff

Antoinehoff commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

I don't think it's a good idea to rewrite everything like this but if everyone wants to pursue well I'll live with it. I'm just not seeing the benefit vs. the risks of messing up with our code base. I don't see why this is needed. I don't care if the writing style is not perfectly strict everywhere and I like the method of just following the guidelines with more recent PR. Style variation also gives us an intuition about how old some parts of the codes are. But I guess standardization and dehumanization are trendy nowadays 🤖

@manauref
Change clang-format option so arguments don't align with opening pare…
74506a1 
…nthesis/brace. As always, it's not perfect, but maybe this is better. See your favorite source file and reg test if you are curious.
@manauref
Use BlockIndent alignment after braces, remove extra white space in s…
9a75fd2 
…truct inits, and add curly braces everywhere. This is a bit closer to what we are used to seeing already.
Maxwell-Rosen
Maxwell-Rosen approved these changes Sep 8, 2026
View reviewed changes

@Maxwell-Rosen Maxwell-Rosen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I went over this with Mana and I love it. Looks great to me.

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

Reviewers

@Antoinehoff Antoinehoff Antoinehoff left review comments
@Maxwell-Rosen Maxwell-Rosen Maxwell-Rosen approved these changes
@JunoRavin JunoRavin Awaiting requested review from JunoRavin
@tnbernard tnbernard Awaiting requested review from tnbernard
@jjz73 jjz73 Awaiting requested review from jjz73

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@manauref @Maxwell-Rosen @Antoinehoff