LGTM

LGTM

LGTM

LGTM

LGTM

Thanks for the Screenshot, please remember to mark your comments as "Done" once you've addressed them. From what I can tell this looks good.

I think you are missing a PARSE unit test

Thank you for the patch, just some observations

what about (may not be useful but compiles)

Great thanks

Thank you for the patch, was there a github issue for this? Can we just validate that those requirements are covered here too while we are at it?

We get heavily criticized for changing defaults between versions, I think if someone want to use GNU style, and they choose to now have this option set it should be done so in their own .clang-format file

If we remove it from here, then it will get removed from the documentation

Oh gosh! It hard to make such changes and someone else not call them a regression, I’m not sure how I feel. We do kind of have to be able to fix bugs and if this gets closer to gnu style then I guess it’s better

nice!

LGTM

@stasm thank you for taking it on, "Commandeer" the revision via the Revision actions.

LGTM

LGTM (sorry I've been slow on the reviews, my day job keeps getting in the way ;-))

LGTM too.. @owenpan this is one of my favourite features!!

This looks good to me, but let the others chime in..

Brilliant thank you, LGTM

You add significant number of keywords here but I don't see any of them being tested? can you add a unit tests to cover what you are adding

Is there any way you could add a unit test for this?

As first passes for adding a new language I think this looks pretty good.

Thanks for taking this on.

Accepting but please wait for @owenpan

The hard part about doing 2 separate cases is where we miss-interpret a .hpp or .h file as being the wrong language (C/C++/ObjC/ObjC++), I don't think I have a problem with a single regex (its not like we change it that often).

LGTM

LGTM thank you for the patch

In D121451#3402167, @curdeius wrote:

In D121451#3401947, @MyDeveloperDay wrote:

I'm a little uncomfortable with

//#<preprocessor directive>

becoming

// #<preprocessor directive>

At least without an option for it to not make changes

Don't you think that //#PPdirective should be indented the same way as other code?
E.g. in:

#include <header.h>

int something;

You comment the whole block and it becomes (with this patch):

// #include <header.h>
//
// int something;

whereas it was:

//#include <header.h>
//
// int something;

which seems inconsistent at least.

I'm a little uncomfortable with

I hit something like this today with some code which was

FWIW I'm not a fan of the while( \n case, so assuming this fix, fixes that then that would be good I think.

It turned out this patch does change behavior.

In D121756#3398165, @sstwcw wrote:

It turned out this patch does change behavior.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

So what do I do?

And what's the regression suite?

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

I think you can spin it any number of ways, but bottom line its a massive truth table and I sort of feel however you change it, it will just become a different equally incomprehensible pattern.

LGTM

Please wait for the other reviewers if you have commit access.

I think fundamentally this look ok? @curdeius and @owenpan I feel I want to bow to your better judgement.

In D121758#3389181, @sstwcw wrote:

Yes. I am surprised that you asked since everyone asked me to break it apart.

Sorry I don't believe we've covered the changes with unit tests.

This just made a 300 lines function and a 500 line function with minimal comments into a 800 line function.. For no real benefit?

Can you please supply full context diff "Context not available."

Wow! Now that’s what I’m talking about!! Awesome! Thank you so much!

I’m not blocking it just using the observation to highlight that the rst generated isn’t conformant with best practices. The solution being that a volunteer offers to fix the dump_format_style.py or enduring Format.h isn’t making those mistakes

So out of interest, what is the reason? my assumption is that you wanted to add more for Verilog and you felt adding the extra bools was the wrong design and its better an an enum

In D121890#3389171, @sstwcw wrote:

The issue is #54418.

Actually I don't know who I should add as reviewers. The patches for clang-format don't always seem to have the same reviewers. Who are the main developers and how do you decide who you add each time?

I give up making the doc generated. It would require either integrating into the build process or writing a parser script like for the style options. Besides, that file is not entirely generated.

In D121757#3389218, @sstwcw wrote:

For the new stuff I have the option of still adding the function parseIndentedBlock but only using it in new code. Please be more blunt about whether I should close this revision and do it that way. I guess I might have misunderstood you before from how you reacted when I closed the large patch.

Just an observation, I have this in my Day Job all the time..

Abandoned? huh? are you breaking it apart?

1. Please log a github issue showing us what you are fixing.
2. Please add the main clang-formatter developers as reviewers and not the clang-format project (I think we see it but I like to see the individual reviewers so I know there opinion)
3. How about you change this so it autogenerates the rst so its never out of date.

Can you add a summary as to why you are making this change? the code doesn't look the same before/after so whats changing and why? is there no unit tests that perhaps need to be added?

You need to rebase the patch, it doesn't apply.

I'm very much a "clang-format all the things" kind of person which is why I added C# and JSON support, as someone who moves between languages all day every day I want one tool to rule them all.. I'd love it if it would format YAML (but thats another story)..

Firstly thank you for taking the time to submit the patch, I've only taken a cursory view at first and I definitely have some comments, I've added the main clang-format reviewers

Please do, to be honest I spent enough time on this and I got to the same conclusion, it needs a rethink. Please go ahead, I don't use this feature myself so I'm not likely to see all the issues.