In D151761#4400056, @galenelias wrote:

In D151761#4394758, @MyDeveloperDay wrote:

did you consider a case where the case falls through? (i.e. no return)

"case log::info :    return \"info\";\n"
"case log::warning :\n"
"default :           return \"default\";\n"

That's a great point. I didn't really consider this, and currently this particular case won't align the case statements if they have an empty case block, however if you had some tokens (e.g. // fallthrough) it would. It's not immediately clear to me what the expectation would be. I guess to align as if there was an invisible trailing token, but it's a bit awkward if the cases missing a body are the 'long' cases that push out the alignment. Also, I don't think it's possible to use AlignTokens and get this behavior, as there is no token on those lines to align, so it's not straightforward to handle. I guess I'll be curious to see if there is feedback or cases where this behavior is desired, and if so, I can look into adding that functionality later. Since right now it would involve a completely custom AlignTokens clone, my preference would be to just leave this as not supported.

LGTM

did you consider a case where the case falls through? (i.e. no return)

Apart from the documentation this looks fine.. thats probably outside the scope of this change

LGTM

I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.

We don't normally land broken tests, even if they are disabled, its better for us if we get a fix at the same time ;-)

Thank you for the indepth explaination in https://github.com/llvm/llvm-project/issues/59178, that was really helpful for me trying to understand what the problem was.

no concerns from the clang-format front.

Interesting

ok! I'm not a massive lambda expert, but aren't we passing by const reference e.g. const FormatStyle &Style, can someone give me a lession as to why it thinks its by value? maybe I'm looking at the wrong "pass by"

Can you link to the Coverity issue so we can see what it was complaining about?

Thanks for the patch...this tells me people are starting to use this feature in anger!! i.e. your formatting via git-clang-format (which is brave!) ;-) which means you have the code modifying features perhaps on my default...

Just an update on this, it works well but some of the cases are not tolerant to comments, I'm working my way through those issues

Could you add anotator tests?

is it possible to pass other arguments like -n if not running in place is there any protection from prevent stdout/stderr from overlapping..?

Handle the case

There is another case I need to cover

Address review comment, add convenience functions to simplify conditions

LGTM

In my C# project, these settings give me just what we tend to use.

Sorry had a clang-format reflow comments moment, revert the unrelated comment changes
simplify the if's a little

Adds additional options to give us much greater control over how we format C# properties especially auto properties

There is more to this than meets the eye.. what we have so far, from existing AfterFunction use and the propsed here AfterCSharpProperty is...

upload the correct patch file

add init support and fix indentation issue when only one property is defined but the is an auto-property

public Foo {
    set;
    get;
}

At least from my experience, the getter is specified before the setter, though I'm unsure how important this is in your eyes.

need to handle init;
need to handle public|private|internal accessors

Address review comments, still would like to solve the indenting issue.

simplify negative if

Regarding the comment that I must not change existing tests: I think this rule is too strict, because those tests are mostly regression tests.
But a regression tests does not test for correctness. So if a test had already a wrong assumption, it must be changeable.

Cloud you include a test that contains multiple levels of nested scope, I'm assuming we won't add an additonal line at every {} level (or will we?)

I know it might not seem an obvious use case but there really isn't a requirement to not include header files more than once.. imagine if I have

@exv any thoughts on this one? given that you were the author of D101860: [clang-format] Fix C# nullable-related errors and
@jbcoe as you were the author of D75983: [clang-format] Improved identification of C# nullables

The exclusions are not complete for example cond? foo() : "B"; would still fail. but this moves us a little closer

re clang-format

for default set;get or get;set for when AfterCSharpProperty is true,

I'm not convinced AfterCSharpProperty is a good name, open for suggestions

If this was the functionality pre 16 then LGTM

In D147176#4266496, @owenpan wrote:

In D147176#4265130, @MyDeveloperDay wrote:

we should be good now

Thanks! I really like these new rules!

In D137327#4267239, @thieta wrote:

In D137327#4235255, @MyDeveloperDay wrote:

In D137327#4234463, @thieta wrote:

This was released in LLVM 16.0.0.

The prior behaviour was there before, it’s marked in GitHub as a regression, can you please revert, we’ll mark the issue to be cherry picked, then let’s go back and rework a solution that means your issue can be resolved

Sorry this slipped under my radar. Can you please push a revert to main and then file a issue to backport this to the release branch if it's something you still want to do. It's hard for me to keep track of the issues unless they are added to the 16.x milestone.

we should be good now

In D147176#4263621, @owenpan wrote:

@MyDeveloperDay We keep getting "spammed" by [libc++][format] patches. Can you delete the rule in H987:

"field": "differential.revision.title",
"condition": "contains",
"value": "[Format]"

or change "contains" to something equivalent to "starts with"?

LGTM

is it possible to point to a github issue that this related to in the review summary, if not please can you make one so we can track the issue its trying to solve

If all comments and concerns are done, then I'm inclined to accept, but I'd like @owenpan and @HazardyKnusperkeks to give their opinion before we land this.

RFC https://groups.google.com/g/llvm-dev/c/wka1Bnrd-aU?pli=1

Yes I didn’t see them repeated below

These features tend to have an additional performance penalty that not all may want to pay, even if they did base their style on your style. I.E. chromium isn’t likely the only ones using chromium style

This was the agreement reached when we pushed for clang format to start being allowed to add or rearrange tokens, I.E. in this case we add {} as such we could, as you saw yourselves make mistakes if our assumptions are not 100% correct and break code, we prefer people opt into this by turning it on in your .clang-format file

In D93240#4254439, @sstwcw wrote:

A goto label isn't affected by this option. Is it intentional?

Sorry I don’t get how this change helps. Removing the option values doesn’t make it clearer IMHO