LGTM

please simply remove line 249

@klimek responded via the mailing list

When I spoke with one of the code owners, they seemed to have a problem accepting this review based on there not being a general description/understanding of how this algorithm works.

LGTM

In D61281#1488022, @RKSimon wrote:

In D61281#1485833, @MyDeveloperDay wrote:

Did this cause some issue? Does this fix something if so can we add a test, because maybe the line isn't needed

I would think we'd want to keep this as an identifier. we are just treating arg? the same as we would arg

@MyDeveloperDay didn't you write this code in D58404?

LGTM

Thanks for the patch, thanks also for changing to https as I believe this was a change suggested in clang tidy docs too

Thanks for the patch, is this case only limited to *<tab>

Did this case some issue? Does this fix something if so can we add a test, because maybe the line isn't needed

LGTM

LGTM

This looks logical to me, seems to fit with what `WhitespaceManager::appendNewlineText` is doing

Abandoning in favor of D60853: clang-format converts a keyword macro definition to a macro function

LGTM

@klimek one possible solution to this might be to replace the "keyword" back to an identifier in a '#define <keywoord>' scenario

use endsWith() as it ignored comments

I suppose we could keep the names and directory structure and just change the namespace. That would just be a special case in the scripts. Haven't looked into it yet, but will do so as soon as I can.

Isn't that matching done on strings? I.e. is there difference between -llvm-* and -ll* ?

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

are we supporting "-llvm-*" in existing .clang-tidy files?

In D60363#1459297, @lebedev.ri wrote:

Please either subscribe the correct lists or set the correct repo in differential params.

Addressing review comments
Reduce test cases

In D60363#1458034, @klimek wrote:

+1 to quite some overlap in the tests. I'd have expected three test cases:

1. ; line comment
2. ; C-comment
3. no semi

@ownenpan might be worth checking with @reuk who has been adding some options for the JUCE style guide and looking at the JUCE code it seems this style might match their style.

In D59087#1456129, @krasimir wrote:

I believe there is no such thing as an "short else statement". The else is part of the if statement and if it is present, I don't consider the whole if statement short. As such, IMO the bug is invalid.

In D60374#1457705, @owenpan wrote:

In D60374#1457693, @MyDeveloperDay wrote:

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

Thanks! I just got the alternative patch ready. Should I discard it and just commit this one?

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

maybe add the following as a test because I think it shows the inconsistency

LGTM (minor nit)

In D60320#1457568, @reuk wrote:

I updated ClangFormatStyleOptions.rst by hand, is there a way to do that automatically instead (for future patches)?

@owenpan can I help you bring this back up to date and review, I think I saw another issue in the bugzilla requesting this.

Thanks for doing this, it LGTM, I like your solution better ;-)

I actually also tend to change doc/ClangFormatStyleOptions.rst too

just a few nits, apart form that it LG

friendly ping

friendly ping

In D59932#1446533, @JonasToth wrote:

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

We normally add something to the documentation about the checker and/or the release notes to say what had changed

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.