run clang-format over vtfontcvt.c

[emaste]

Summary

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

[emaste]

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]

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)

[imp]

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.

[imp]

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.

[emaste]

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

[imp]

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]

vtfontcvt.c

437 ↗

(On Diff #76696)

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

[gallatin]

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

[emaste]

In D26340#585922, @gallatin wrote:

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.

[emaste]

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

[arichardson]

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.

[emaste]

vtfontcvt.c

31–32 ↗

(On Diff #76696)

I suppose an 11.0 regression then

$ clang-format --version
FreeBSD clang-format version 11.0.0 ([e-mail]: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.

[imp]

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.

[arichardson]

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.

[emaste]

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.

[arichardson]

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

[arichardson]

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).

[arichardson]

Looks slighlty better with aligned comments: https://reviews.freebsd.org/differential/diff/76877/

[emaste]

updated diff from @arichardson

[emaste]

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

[arichardson]

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]

usr.bin/vtfontcvt/vtfontcvt.c

436

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

[gallatin]

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.

[imp]

In D26340#587230, @gallatin wrote:

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.

[emaste]

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).

[arichardson]

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.

[emaste]

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?

[arichardson]

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.

[emaste]

Run again with current .clang-format and Clang 11

[emaste]

usr.bin/vtfontcvt/vtfontcvt.c

304

"Casts and sizeof's are not followed by a space" - this is a valid fix from clang-format

679–681

I'm surprised by this one

685

"Casts and sizeof's are not followed by a space" but it seems much of our existing code implicitly adds "except (void) casts to intentionally discard a return value are followed by a space." IMO this is not worth trying to support and we should just drop those spaces.

855

valid fix

1031–1035

We can /* clang-format off */ to leave these as is if desired.

[arichardson]

usr.bin/vtfontcvt/vtfontcvt.c

679–681

Clang-format 12 should do the right thing here: https://reviews.llvm.org/D90246

[emaste]

usr.bin/vtfontcvt/vtfontcvt.c

436

We've talked a bit about having submitters use git format-patch (from llvm); of all clang-format I am aware of I think this one here is the only one that stands in the way of that.

679–681

Confirmed, this is not changed now w/ clang-format12

[imp]

usr.bin/vtfontcvt/vtfontcvt.c

436

We've talked about telling submitters to submit style(9) compliant patches, and to use git clang-format when in doubt.

git format-patch is something different that just prints the patches as an email.

458–459

I find this reformatting gratuitous. Then again, I want to relax the width to 100.

484

This is an extra indent, but there's already a bug upstream.

508

This is also wrong: there's an extra space to align it with the ( which isn't something style(9) does.

651

This is also wrong: this space shouldn't be there. We don't do this in sys/kern at all, for example.

685

style(9) should be changed to allow the space, but not require it so clang-format can do either and we're fine either way.

862

This is wrong. In addition to the normal "shouldn't have moved this from the end of the end" this compounds the problem by not collapsing the strings. But it's another example of why 100 would produce clearer code. Though given clang-format's penchant for always packing things optimally (which I'd really like to suppress), a change to 100 would introduce more churn...
It also has the extra indent stuff...

[emaste]

usr.bin/vtfontcvt/vtfontcvt.c

436

Yes, brain was decoupled from my fingers, I meant git clang-format

458–459

I agree, I prefer the original version here. But if someone ran clang-format over their submitted patch and it produced the new version I'd be fine leaving it

651

I might argue this is a bug in style(9), in that the queue macros should be syntactically similar to for()

[imp]

usr.bin/vtfontcvt/vtfontcvt.c

458–459

OK. In the context of 'is this wrong if we got a patch like this' likely no.
In the context of 'should we force this' the answer is also no.
In the context of 'should we reformat lots of things while clang-format does this'? the answer is maybe in some select cases, but generally no especially for CAM code where this messes with existing practice too much.

651

style(9) generally is supposed to document what's in the tree so that new submissions fall within an acceptable range.
As such, I could find 3991 instances of '_FOREACH(' and 2 instances of '_FOREACH ('. As far as documenting what's actually
done goes, this is better than most. While it kinda is like a for statement, and there's a certain aesthetic appeal to changing it,
I'd say that pragmatism trumps aesthetics, especially since emacs auto-formats it this way given the usual style-9 settings people have/use.