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 Skipped

Unit

Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

emaste added inline comments.Sep 5 2020, 7:15 PM

vtfontcvt.c
437 ↗	(On Diff #76696)	Not sure why it got this wrong
459–460 ↗	(On Diff #76696)	original version seems preferable

emaste edited the summary of this revision. (Show Details)Sep 5 2020, 7:25 PM

emaste added a subscriber: andrew.Sep 5 2020, 7:44 PM

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.

emaste added inline comments.Sep 5 2020, 9:14 PM

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 added inline comments.Sep 5 2020, 9:24 PM

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 a subscriber: freqlabs.Sep 5 2020, 10:56 PM

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?

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.

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

arichardson added inline comments.Sep 8 2020, 9:25 AM

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 added inline comments.Sep 8 2020, 12:25 PM

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.

imp added inline comments.Sep 9 2020, 10:12 PM

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 added inline comments.Sep 10 2020, 1:01 PM

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 added inline comments.Sep 10 2020, 1:09 PM

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

arichardson added inline comments.Sep 10 2020, 1:31 PM

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 mentioned this in D26391: Set AlignTrailingComments in the clang-format config.Sep 10 2020, 1:38 PM

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

arichardson mentioned this in rS365576: Set AlignTrailingComments in the clang-format config.Sep 10 2020, 2:11 PM

updated diff from @arichardson

Harbormaster completed remote builds in B33487: Diff 76877.Sep 10 2020, 2:28 PM

emaste added inline comments.Sep 10 2020, 2:31 PM

usr.bin/vtfontcvt/vtfontcvt.c
106–108 ↗	(On Diff #76877)	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 ↗	(On Diff #76877)	these ones are the case I really want to see fixed

arichardson added inline comments.Sep 10 2020, 3:33 PM

usr.bin/vtfontcvt/vtfontcvt.c
436 ↗	(On Diff #76877)	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 a subscriber: cem.Sep 11 2020, 1:21 AM

cem added inline comments.

usr.bin/vtfontcvt/vtfontcvt.c
436 ↗	(On Diff #76877)	The latter. We don't indent by paren-depth, just by continuation or brace (if/for/etc).

cem removed a subscriber: cem.Sep 11 2020, 1:21 AM

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.

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.

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 added inline comments.Sep 24 2020, 1:41 PM

usr.bin/vtfontcvt/vtfontcvt.c
220 ↗	(On Diff #76877)	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 ↗	(On Diff #76877)	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 added inline comments.Sep 24 2020, 2:46 PM

usr.bin/vtfontcvt/vtfontcvt.c
220 ↗	(On Diff #76877)	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 added inline comments.Sep 24 2020, 2:49 PM

usr.bin/vtfontcvt/vtfontcvt.c
220 ↗	(On Diff #76877)	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.

Run again with current .clang-format and Clang 11

emaste added inline comments.May 13 2021, 12:15 AM

usr.bin/vtfontcvt/vtfontcvt.c
304 ↗	(On Diff #89097)	"Casts and sizeof's are not followed by a space" - this is a valid fix from clang-format
679–680 ↗	(On Diff #89097)	I'm surprised by this one
685 ↗	(On Diff #89097)	"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 ↗	(On Diff #89097)	valid fix
1031–1035 ↗	(On Diff #89097)	We can `/* clang-format off */` to leave these as is if desired.

emaste mentioned this in D30237: vtfontcvt: minor style(9) fixes from clang-format.May 13 2021, 12:22 AM

arichardson added inline comments.May 13 2021, 10:23 AM

usr.bin/vtfontcvt/vtfontcvt.c
679–680 ↗	(On Diff #89097)	Clang-format 12 should do the right thing here: https://reviews.llvm.org/D90246

emaste added inline comments.May 13 2021, 1:31 PM

usr.bin/vtfontcvt/vtfontcvt.c
679–680 ↗	(On Diff #89097)	Confirmed, this is not changed now w/ clang-format12
436 ↗	(On Diff #76877)	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.

imp added inline comments.May 13 2021, 1:50 PM

usr.bin/vtfontcvt/vtfontcvt.c
458 ↗	(On Diff #89097)	I find this reformatting gratuitous. Then again, I want to relax the width to 100.
484 ↗	(On Diff #89097)	This is an extra indent, but there's already a bug upstream.
508 ↗	(On Diff #89097)	This is also wrong: there's an extra space to align it with the ( which isn't something style(9) does.
651 ↗	(On Diff #89097)	This is also wrong: this space shouldn't be there. We don't do this in sys/kern at all, for example.
685 ↗	(On Diff #89097)	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 ↗	(On Diff #89097)	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...
436 ↗	(On Diff #76877)	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.

emaste added inline comments.May 13 2021, 2:20 PM

usr.bin/vtfontcvt/vtfontcvt.c
458 ↗	(On Diff #89097)	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 ↗	(On Diff #89097)	I might argue this is a bug in style(9), in that the queue macros should be syntactically similar to for()
436 ↗	(On Diff #76877)	Yes, brain was decoupled from my fingers, I meant `git clang-format`

domagoj.stolfa_gmail.com mentioned this in D30248: run uncrustify over vtfontcvt.c.May 13 2021, 3:16 PM

imp added inline comments.May 13 2021, 9:12 PM

usr.bin/vtfontcvt/vtfontcvt.c
458 ↗	(On Diff #89097)	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 ↗	(On Diff #89097)	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.