In D33029#1944919, @bbassi wrote:

@MyDeveloperDay Is someone working on fixing the breaking tests and merging it? I need this feature so if someone isn't working on it already, I can take it.

see the discussion at D68969: [clang-format] Remove the dependency on frontendD69854: [clang-format] [RELAND] Remove the dependency on frontendD68554: [clang-format] Proposal for clang-format to give compiler style warnings

We specifically dropped clangFrontEnd from clang-format, please don't include it again

New implementation that breaks fewer tests.

Could the default be Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock

LGTM

nice, thank you.

is there overlap here D76197: clang-format: Use block indentation for braced initializations

I want to believe that it was a mistake on the original developers part, but I just can't tell.

Not sure if tagging is considered rude, I figure that @MyDeveloperDay's notification fell off your radar.

@MarcusJohnson91 To get a review past its better that you mark the comments as done so the reviewers know when the comments have been addressed and not missed. (this is important as the number of comments grows)

your patch seems to be missing the other files

you need documentation and release note changes too

@krasimir, well I learned something new, so thanks for taking the time to explain that to help improve our understanding.

This feels better than merging

This needs a full context diff, before we can even look at it properly.

LGTM, thank you for adding the extra test, please mark the inline comments as done

LGTM

LGTM

I tend to agree with your assessment, I think its worth waiting for some others to comment.

I think its look good, please add the extra tests, then lets give people a couple of days

Is this intentional, or is it an oversight?

you need to add any diff as a full context diff (normally by adding -U 9999999) to the diff command

If you are changing unit tests then I'm nervous, imagine if this many tests change what the impact is on a large code base. I think we need to understand more about why you are making this change and why you think its ok to change/remove existing tests which assert some behavior which up to now we have considered correct.

You need to add unit tests

This LGTM, thank you for the patch

maybe a unix2dos thing going on on the release notes

Correct follow that description on how to request commit access

This patch is nearly 2 years old, and there has been a lot of discussion and I can see you've put a lot of work into adding the tests which prove it works, from my perspective this LGTM as long as you are prepared to help resolve any issues that might come up as a result of this change.

LGTM, thanks

In the absence of any other comments, This LGTM, this may help to resolve D68569: [clang-format] Also look for .{ext}.clang-format file, I think I prefer this solution.

LGTM, thanks for the patch (I've seen this in the wild where the AssemblyInfo.cs gets a bit messed up.

Nit: please give the revision a description

In D73354#1854115, @mprobst wrote:

This has landed as https://reviews.llvm.org/rGa324fcf1ae6 (not sure why this hasn't closed automatically).

Did you mean to commit D73354: clang-format: insert trailing commas into containers. too?

LGTM thank you

Nit: don't forget the ClangFormatStyleOption.rst and ReleaseNote.rst changes

I'm happy if you are, I think given that you've really been one of the main contributors of clang-format [JS] for the last number of years you are the best person to probably assess the impact on Javascript code.

these are great patches you are making...

Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst (if there are any changes because you modified Format.h).

Nit: Could you add an entry for ReleaseNotes.rst and regenerate the ClangFormatStyleOption.rst with the docs/tools/dump_style.py

Just for reference, take a look at D58404: [clang-format] Add basic support for formatting C# files especially the historical diff

This looks good, I had lots of problems when I first started the C# support around this which the initial C# support tried to address by adding support in the lexer, but we pulled back from that implementation. So its no surprise to me we are hitting a few errors in this area, one issue I've had problems with is a string ending with \ e.g.

FYI maybe related D33029: [clang-format] add option for dangling parenthesis

I'm a little uncomfortable about all the tests changing, and I'm unsure if we should be changing the default behaviour.

In D72144#1834265, @jbcoe wrote:

Removed test snippets that did not exercise new code.

@krasimir Thanks for fixing this, as it looks like I broke it.. its much appreciated

I think if you can fix the missing documentation it looks pretty good to go.

Please can you regenerate the ClangFormatStyleOptions.rst using docs/tools/dump_style.py and add an entry into the clang release notes in the clang-format section

In D71512#1815512, @Bouska wrote:

Ping @MyDeveloperDay @klimek

In D72793#1828625, @timwoj wrote:

I know that 10.0 was branch recently. Any chance this can make it over?

I need you to rebase this fix, also you need to clang-format it as well. Format.h/ContinuationIndenter.cpp and FormatTestObjC.cpp all failed clang-format check

LGTM

LGTM thanks for the patch

In principle, I think this is something that might help a lot of people as I've seen people asking for a better mechanism to be able to load a centrally stored .clang-format file.

LGTM, thanks once again for the C# patch!

This LGTM, thanks for the patch