Page MenuHomeFreeBSD

run clang-format over vtfontcvt.c
Needs ReviewPublic

Authored by emaste on Sep 5 2020, 7:08 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Posted as an example of running clang-format on some file in our tree, to see what it gets right/wrong/indifferent.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 33487
Build 30763: arc lint + arc unit

Event Timeline

emaste requested review of this revision.Sep 5 2020, 7:08 PM
emaste created this revision.
emaste added inline comments.
vtfontcvt.c
31–32 ↗(On Diff #76696)

This seems undesired

437 ↗(On Diff #76696)

Not sure why it got this wrong

459–460 ↗(On Diff #76696)

original version seems preferable

emaste added inline comments.
vtfontcvt.c
35 ↗(On Diff #76696)

@andrew pointed out we need only one of sys/types.h and sys/param.h
(probably not something for clang-format to care about)

meh, not a very good job. I see only one actual formatting bug in all these changes, though the headers are arguably slightly better modulo the double inclusion.

vtfontcvt.c
57 ↗(On Diff #76696)

this sort of change will be a huge source of churn in a number of areas. Some files do the alignment, others don't, many others are mixed. There's no benefit from this :(

107 ↗(On Diff #76696)

these were lined up on purpose and look better that way.

185 ↗(On Diff #76696)

meh, this arguably violates style(9) which encouraged more things on one line.

309 ↗(On Diff #76696)

these two changes are the only beneficial changes in the file.

760 ↗(On Diff #76696)

these are wrong too

829 ↗(On Diff #76696)

this is wrong.

1011 ↗(On Diff #76696)

channeling bde: these were purposely outdented.

One of the big advantages of style(9) has always been it's been a range of acceptable behavior. As we've loosed the strict dogma over the years, it provided a large enough space that people stopped fighting over style issues... Going to a strict, at checkin time, thing would a huge step backwards. clang-format fixes a few issues, but still causes far more issues than it fixes. It's OK if someone wants to run it on their code that has a radically different format, but for things that are close, I don't see the value.

vtfontcvt.c
57 ↗(On Diff #76696)

If we're fine with either version there's a benefit from removing cognitive overhead from dealing with this.

185 ↗(On Diff #76696)

I'm not quite sure what clang-format's intent here is -- string arguments should start on their own line?

760 ↗(On Diff #76696)

Yes these are all instances of the same case

vtfontcvt.c
57 ↗(On Diff #76696)

I'm fine with allowing either version. Im not fine picking one and forcing the whole tree to conform. I've written both styles and think there are time and place for each...

185 ↗(On Diff #76696)

Yea, it seems nuts

freqlabs added inline comments.
vtfontcvt.c
437 ↗(On Diff #76696)

Looks like it indented four spaces for if and another four for sscanf

What command line arguments, or other clang format configuration was used here?

What command line arguments, or other clang format configuration was used here?

Just clang-format -i usr.bin/vtfontcvt/vtfontcvt.c; .clang-format is in the FreeBSD tree already.

Here's another example, running it on the string functions we imported from musl: D26353

vtfontcvt.c
31–32 ↗(On Diff #76696)

This seems undesired

This can probably be worked around with some clang format config tweaks. Will investigate on Monday or submit a patch to upstream if it can't be worked around.

31–32 ↗(On Diff #76696)

I don't see this change after running clang-format -i usr.bin/vtfontcvt/vtfontcvt.c with my local version of clang-format (clang-format version 10.0.1).

63 ↗(On Diff #76696)

I think we probably need a patch upstream if we want to keep the tab after define.

vtfontcvt.c
31–32 ↗(On Diff #76696)

I suppose an 11.0 regression then

$ clang-format --version
FreeBSD clang-format version 11.0.0 (git@github.com:llvm/llvm-project.git llvmorg-11.0.0-rc2-0-g414f32a9e86)
63 ↗(On Diff #76696)

style(9) wants it:

Put a single tab character between the #define and the macro name.

I'm indifferent. Maybe others can comment on the reasoning behind it.

vtfontcvt.c
63 ↗(On Diff #76696)

You got me on this one. Prior to 1999 it was implicit from the tabs in the man page! David O'Brien made it explicit.
It was silently changed from a space to a tab in 1995 by phk in a big rototill of the file where things were indented with indent.
I could find no further explanation, though I know I got lots of define<tab>macro feedback when I first started in the mid 90s.
So long as we accept both styles, I think this might be a 'dust bin of history' item.

vtfontcvt.c
31–32 ↗(On Diff #76696)

I just tried formatting with a recent LLVM git revision and it's formatted as expected. I'll have a look a the git history to see if there is a commit that we should cherry-pick.

vtfontcvt.c
31–32 ↗(On Diff #76696)

Ok, interesting. I will try to test on a new machine also, to double-check that I didn't have some local change.

Looks almost the same with latest LLVM git just without the __FBSDID() weirdness: https://reviews.freebsd.org/P421

vtfontcvt.c
56–61 ↗(On Diff #76696)

If you set AlignConsecutiveDeclarations: true in the clang-format you get the following.

But then it also alings all delcarations inside functions which I don't think is desirable. I also find the
uint8_t * g_data; a bit odd, so it's probably better not to align (unless someone wants to fix clang-format).

usr.bin/vtfontcvt/vtfontcvt.c
106–108

Alignment applies to consecutive lines of comments, each new block gets its own alignment. This is probably the best that can be expected from a tool and I think it's a win over the previous version, even if not perfect.

436

these ones are the case I really want to see fixed

usr.bin/vtfontcvt/vtfontcvt.c
436

Clang-format currently indents each new opening paren by 4. Should this be handled differently for if/for/while statements and function calls?

Or do we want something like

int x = fn1(fn2(inner_arg1,
    inner_arg2), outer_arg2);

where a continuation line is always indented by 4 regardless of how many nested function calls there are?

cem added inline comments.
usr.bin/vtfontcvt/vtfontcvt.c
436

The latter. We don't indent by paren-depth, just by continuation or brace (if/for/etc).

Is there a file which is already perfectly style(9) compliant? Is it this file?

Assuming a "perfect" file, we could then tweak the clang-format config iteratively, until we find the output which has the smallest diff, and then tweak clang-format to fix the remainder and bring the diff to 0.

Is there a file which is already perfectly style(9) compliant? Is it this file?

Assuming a "perfect" file, we could then tweak the clang-format config iteratively, until we find the output which has the smallest diff, and then tweak clang-format to fix the remainder and bring the diff to 0.

The problem with this approach is that style(9) isn't canonical, but is a range of allowed styles. It would be useful to iterate, but there will be limits that are important to keep in mind.

Is there a file which is already perfectly style(9) compliant? Is it this file?

I picked this one only because I'm familiar with it, and knew it was mostly style(9) compliant with some intentional deviations (like print_font_info).

usr.bin/vtfontcvt/vtfontcvt.c
220

How do feel about space vs no space for the foreach macros?
It seems like there is already a clang-format option for this: SpaceBeforeParens: ControlStatementsExceptForEachMacros vs SpaceBeforeParens: ControlStatements (default).

My guess is that most don't have the space, but I haven't checked.

436

I had a quick look into fixing this but it seemed non-trivial, so I've filed https://bugs.llvm.org/show_bug.cgi?id=47635 for now.

usr.bin/vtfontcvt/vtfontcvt.c
220

I'd also guess most don't have the space, but it seems to me that these should be treated the same as regular for loops. We call them out explicitly as ForEachMacros in .clang-format; I guess that without doing so they would have no space, but the { would also move to the next line?

usr.bin/vtfontcvt/vtfontcvt.c
220

Yes, they would be formatted like a function call with a missing semicolon and the braces then become a plain scope rather than part of the line before.