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

update clang-format #6440

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

Conversation

garlick
Copy link
Member

@garlick garlick commented Nov 14, 2024

Salvage the improvements to our .clang-format file from #4694 which was out of date.

trws and others added 4 commits November 14, 2024 10:12
Problem: .clang-format is out of sync with project style.

This uses some newer options (clang 11) to get it a little closer.

Originally proposed in flux-framework#4694
Problem: the column limit of 88 chars is inconsistent with
current project practices.

Set the limit to 80 chars.
Problem: a comment about requiring clang-3.6 or clang-7 is
probably no longer useful at this point as those releases
are now old.

Drop comment.
Problem: the current clang-format removes alignment of the
values of consecutive macro assignments, which makes things
less readable.

clang-20 adds AlignConsecutiveMacros.
Add it so we'll be ready.
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.65%. Comparing base (ae1f516) to head (f5484bb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6440   +/-   ##
=======================================
  Coverage   83.64%   83.65%           
=======================================
  Files         524      524           
  Lines       87693    87693           
=======================================
+ Hits        73355    73358    +3     
+ Misses      14338    14335    -3     

see 10 files with indirect coverage changes

@trws
Copy link
Member

trws commented Nov 14, 2024

I see you decided to re-restrict the line length to 80. While we do generally try to hit 80 we don't actually follow it everywhere but clang-format will enforce it. Running a little script to get line lengths of every C source file in src/, filtering out libev and some of the other external things we vendor. There are 466 lines that are between 80 and 88 lines long, and 540 more that are above 88 characters long. A lot of the longer ones (especially over 100 chars) are in the tests, so that mostly doesn't count, but here are the files impacted by at least one line between 80 and 88 characters:

src/cmd/flux-logger.c
src/common/libflux/conf.c
src/common/libflux/test/rpc.c
src/common/libflux/test/rpc_security.c
src/common/libhostlist/test/hostlist.c
src/common/libidset/test/idset.c
src/common/libjob/test/job.c
src/common/libjob/test/sign_none.c
src/common/libjob/test/unwrap.c
src/common/libkvs/test/treeobj.c
src/common/liboptparse/test/optparse.c
src/common/libpmi/test/canonical2.c
src/common/librlist/test/rhwloc.c
src/common/librouter/test/router.c
src/common/libsubprocess/test/channel.c
src/common/libsubprocess/test/fbuf_watcher.c
src/common/libsubprocess/test/stdio.c
src/common/libtaskmap/test/taskmap.c
src/common/libutil/basemoji.c
src/common/libutil/sha1.c
src/common/libutil/sha256.c
src/common/libutil/sha256.h
src/common/libutil/test/cronodate.c
src/common/libutil/test/sha256.c
src/common/libutil/test/stdlog.c
src/common/libutil/timestamp.c
src/modules/content-s3/content-s3.c
src/modules/content-s3/s3.c
src/modules/job-list/state_match.c
src/modules/job-list/test/match.c
src/modules/job-manager/test/job.c
src/modules/kvs/test/kvstxn.c
src/modules/kvs/test/lookup.c
src/shell/test/jobspec.c

FWIW, the linux kernel also requests a line length of 80 characters, but changed checkpatch and other tooling to only force format or error at 100 characters. That combined with our black line limit being set to 88 was why I had it as 88 to begin with.

To be honest I don't really mind, but this is where that came from.

@garlick
Copy link
Member Author

garlick commented Nov 14, 2024

From RFC 7:

Lines SHOULD be limited to 80 characters.

Which implies a best effort, but clang-format will actively remove line breaks to expand into 88 characters which undoes our best efforts.

We could argue that 88 should be the number in RFC 7 but that would be a PR on the RFC. Here we should follow the rule.

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

Successfully merging this pull request may close these issues.

2 participants