LGTM

@lebedev.ri Are we talking about a general ideology of the long term cost to allow any new things in? or to not allow things in this specific case?

Address comment nits

LGTM

Addressing Review Comments

• Remove all non Format only code (i.e. changes to clang/Basic and clang/Lexer to support verbatim interpolated strings C# 6)
• For the most part the need to for them is removed by merging of tokens during the FormatLexer phase
• There is one obscure failure scenario, but for a first pass I think it can be ignored (tests commented out) where a \ is used unescaped just before a " (because of paths can be more common than you think)
  • string s = @"ABC\" + ToString("B");";

The cost is financial, as it's developer time, which costs real money to companies. In the end, to support this, people like myself who are doing this as part of their job spend time that they'd otherwise spend to make other things better that might be more important.

Sorry for the spam:

• write this a little better

Addressing code review comment

@klimek I think is what you meant, a much smaller and neater version of what I had before.

In D52150#1436423, @to-miz wrote:

I checked out the git repository and applied the patch to head and reran all tests.
The diff produced by git is basically the same, should I upload the new diff?

Address review comments
This may not be a more satisfactory solution but it at least now covers the other cases raised by @JonasToth
Add more tests around this area.

Minor modification to improve the [=] case

In D59540#1434837, @JonasToth wrote:

What happens on [=]() ..., direct capture [&Columns]()... and [Columns]()...?

(landed here too https://github.com/mydeveloperday/clang-experimental/releases)

ping @mprobst,@mitchellwills noticed you just reviewed another clang-format-js commit, wondered if you were interested in reviewing this for me?, finding it hard to get any review traction.

I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason

I'm sorry for not having a positive answer here, but I'm not in favor of this change. The style rule looks like it introduces arbitrary complexity at a point where I don't understand at all how it matters. We cannot possibly support all style guides that come up with random variance. I do not think this option pulls its weight.

I'm happy to be wrong, but In current master, SpaceBeforeCpp11BracedList is a boolean not an enum (but I think I've seen a review changing it somewhere which maybe isn't landed)

nobody being able to make changes

So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.

From what I can tell this looks OK as you seem to be mainly only passing down the boolean, my guess is that while most people don't use the -lines directly that often but it probably gets called by some of the formatting tools that work over selections, maybe something like git-clang-format

Remove non related clang-format changes
run git clang-format
Add a Microsoft style (trying to match most closely the more traditional Microsoft style used for C#)
This will allow, CSharp to be added to the config by

@to-miz do you have permission to commit?

Pulling this revision down to build it caused build issues with missing SBBLO_Always, could you investigate before committing?

In D59309#1428970, @klimek wrote:

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

Should we fix isStartOfName then?

In A<bool> or A<8> A should be TT_StartOfName, too, right?

In D59309#1428970, @klimek wrote:

In D59309#1428969, @MyDeveloperDay wrote:

That works because the argument list is just tok::identifier and TT_StartOfName

Should we fix isStartOfName then?

In A<bool> or A<8> A should be TT_StartOfName, too, right?

of course, this isn't legal

That works because the argument list is just tok::identifier and TT_StartOfName

In D59309#1428799, @klimek wrote:

Why did the numeric constant break this? Which path would the function run through?

sorry incorrect files in the merge reverting...

run git clang-format

most of the cases that we were adding here were for a templated return types

LGTM

Address review comment

• add document and comment spelling and punctuation
• add additional unit tests to show non compound else clause case

Address review comments

• collapse two options into one

In D33029#1423947, @lebedev.ri wrote:

In D33029#1423944, @MyDeveloperDay wrote:

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

In D59087#1423942, @reuk wrote:

Does it make sense to allow AllowShortIfElseStatementsOnASingleLine to be enabled when AllowShortIfStatementsOnASingleLine is not?

Perhaps it would be better to have AllowShortIfStatementsOnASingleLine be represented by an enum with values Never, WithoutElse, Always. The old true/false values from config files prior to this change would need to be made to map to the appropriate enum keys.

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

In D55170#1423798, @reuk wrote:

Thanks for the review, and for approving this PR. It's very much appreciated!

I don't have merge rights - could someone commit this for me please?

Please add unit tests to show this working, otherwise later someone will come along and break the behavior

This review lack unit tests, please add some for FormatTest to show its use cases, otherwise someone else will come along and break this later

This review needs unit tests

I'm not the code owner but this LGTM,

Fix spelling typo in documentation and comment

Add missing Format.h from the review

I definately been burnt by not handling all the cases, but imagine a string class

I'm not sure I personally would ever write code like that ;-) , but if its legal C++ and it compiles we should handle it the same as foo<1>,foo<true>,foo<!true>

In D58922#1417605, @jkorous wrote:

BTW I suggest you also add the original test case since the test cases you added fail the expectation at FormatTest.cpp:74 and the message is a bit unclear whether the testcase or the actual implementation is at fault.

EXPECT_EQ(Expected.str(), format(Expected, Style))
    << "Expected code is not stable";

The original reported case IMO gives a better idea what is broken since it duplicates the end-of-namespace comment.
https://bugs.llvm.org/show_bug.cgi?id=40910

It might be a good idea to add the nested lambda tests from D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style) into your review to show how this patch copes with those (and the examples from the comments of that review)

Thank you for helping to address one of the many clang-format issues from bugzilla, I'm not the code owner here but this looks good to me. If we want to address the many issues we need to be able to move forwards.

Fix negative test case
support the same /*<space>clang-format off<space>*/ in the sort includes that the TokenLexer supports.

This looks good to me but I would wait for one of @JonasToth or @alexfh to perhaps take a look,

sorry to be critical, just trying to help get traction on your review...

I'm trying to go over old reviews and see if they are still desired, I stumbled on your review and want to see if its still important. Here is some initial feedback.