Not sure why it got this wrong

original version seems preferable

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

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 :(

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

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

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

these are wrong too

this is wrong.

channeling bde: these were purposely outdented.

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

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

Yes these are all instances of the same case

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

Yea, it seems nuts

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

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.

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

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

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)

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.

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.

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.

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

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

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.

these ones are the case I really want to see fixed

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?

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

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.

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.

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?

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.

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

I'm surprised by this one

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

valid fix

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

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

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

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.

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

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

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

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

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.

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

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.

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

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

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

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.

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.