wrong patch file

I assume the intention was that users could have DisableFormat=true and SortIncludes=true when they want to sort the includes but not perform any additional formatting in the code.

LGTM

I should have known it was more involved.

when/if you commit can you abandon D6833: Clang-format: Braces Indent Style Whitesmith

Thank you for this patch

Yes, it would be good if it is landed. And can I know the procedure for getting commit access

Does this need landing? given that you have a number of patches in flight perhaps it would be good to request commit access

Thank you for this patch, i've seen a number of PRs raise to this effect.. this looks to be a great start in this area

This LGTM, just some minor Nits

but perhaps I'm really just agreeing to @sammccall 's suggestion of "-style=<filename>" which kind of feels sufficient to me..

I am not opposed to this idea, I actually think this has some mileage based on a use case I've encountered:

LGTM

This patch is needs rebasing

LGTM

LGTM

LGTM

Assuming this works and the other unit tests don't show issues then this LGTM. Please consider running this on your NetBSD code base before committing, if possible please also run on clang code based to ensure existing sorted headers aren't sorted unexpectedly.

We don't normally commit a failing test alone, otherwise the build machines will be broken until it gets fixed.. just add this test with your fix.

I appreciate what you've done to make this patch less duplicated code..but now you've almost got to the point where being able to control it all via configuration.

In D64695#1584740, @Manikishan wrote:

In D64695#1584706, @MyDeveloperDay wrote:

There also seems like alot of duplication between the existing sortCppIncludes

I think the only difference here is really just

SmallVector<unsigned, 16> Indices;
  SmallVector<unsigned, 16> Includes_p;
  for (unsigned i = 0, e = Includes.size(); i != e; ++i) {
    unsigned pl = getNetBSDIncludePriority(Includes[i].Filename);
    Includes_p.push_back(pl);
    Indices.push_back(i);
  }

vs the original

SmallVector<unsigned, 16> Indices;
for (unsigned i = 0, e = Includes.size(); i != e; ++i)
  Indices.push_back(i);

plus way the sorting is performed, are we sure we couldn't have just made the original sorting more powerful based on style settings?

Does it mean that adding the priority to sort based on style?
like this:

if (Style== NetBSD)
   // set priority to netbsd's priority
else if(Style == X)
  // set X's priority

I didn't want to mess up the original sorting and made up this patch, if we have parameterise this solution, I will go for it.

There also seems like alot of duplication between the existing sortCppIncludes

Do you think there is anything about this algorithm that could be parameterized so that other projects could utilize it? I guess its not completely clear how this differs from just using the IncludeCategories?

It was more about not having time to pursue it at this time.. I didn't feel I should hog the PR if someone else wanted to take a look.

Nit: if you going for "OnePerLine" in the name rather than "Break" can you keep it consistent with ConstructorInitializerAllOnOneLineOrOnePerLine and put the type first and then the operation i.e. BitFieldOnePerLine (this will help keep all BitField options together if there are ever more than one), I also think either don't use Decl at all or use the full word Declarations as in AlignConsecutiveDeclarations , AlwaysBreakTemplateDeclarations etc

mark sure you mark off the comments as you consider them done.

You need to add a unit test in clang/unittests

Just a passing comment, Is this:

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?