search

flux-framework / flux-core

core services for the Flux resource management framework
GNU Lesser General Public License v3.0
168 stars 50 forks source link

update clang-format #6440

Open garlick opened 2 weeks ago

garlick commented 2 weeks ago

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

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/flux-framework/flux-core/pull/6440/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flux-framework)
trws commented 1 week ago

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 commented 1 week ago

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.