In D87587#2592605, @kuzkry wrote:

Do you need someone to push it?

Yes, I need someone to do that. I don't have write permissions. I'm new here.

• Work with ::max instead of many checks for == 0.
• Additional Testcase

Is this acceptable to the others? Do you need someone to push it?

Do you need someone to push this?

Ping.

In D97137#2584417, @darwin wrote:

Hi guys, thanks for accepting the change. But I don't have commit access of LLVM. Can someone commit it for me?

In D97137#2580664, @darwin wrote:

In D97137#2579669, @HazardyKnusperkeks wrote:

You should mark comments as done, if they are.

Does your modification maybe add something to the alignment which is not a declaration?

int a;
double b;
a * b;

How is that formatted? Yeah unlikely that something like that is in code, but it could be if operator* has side effects and one does not need the result.

Good question.

I've tested the original code and the modified code, both will generate the same result:

int    a;
double b;
a*     b;

I understand the expected format should be:

int    a;
double b;
a* b;

Maybe we can register another bug to track it.

You should mark comments as done, if they are.

Just out of curiosity, in which language is Const used?

In D95017#2572578, @PragmaNull wrote:

In D95017#2572238, @curdeius wrote:

Do you have an idea for better names?
I see that e.g. MS documentation uses ascending order and case-sensitive order.

I'm okay with the names, it just seems to me that they are reversed, where CaseSensitive should sort using ASCIIbetic order, and CaseInsensitive should ignore case entirely in the same way that most case-insensitive string compares would.

Added DiagHandlerCtxt.

In D96760#2565171, @njames93 wrote:

I did have another plan for a different way to go about this, not sure if its really any better though.
It involved building a vector of the edits the config would apply to its parent. Then after the parent is parsed just applying those edits.
It results in only 1 pass over the config file however it's a little more involved to get it to work.

Although I would put it in one test, this is fine by me.

Have you rebased your change? It seems that your release notes are still for LLVM 12.

Apart from my comments, this looks good to me and I really wonder why it hasn't been pushed. (I would directly use it!)

Silence unused variable warnings in release and using llvm::reverse with range based for.

In D93844#2536354, @njames93 wrote:

As a follow up it may be wise to pass a diag handler to parseConfiguration as when we parse it a second time, we probably want to disregard any warnings (like unknown key) detected as they will have been printed on the first pass.

You need to supply a full diff (with context).
Please also add it to the release notes.

Fixed documentation (and rebased).

In D92257#2532077, @MyDeveloperDay wrote:

I have a script that runs clang-format -n on various directories in clang
that are clang format clean, polly is one of them because they have clang
format as a unit test

I use this to ensure I don’t regress behaviour

Maybe we should formalise this with some sort of clang-format-check cmake
rule

Mydeveloperday

In D69764#2532666, @MyDeveloperDay wrote:

What can be done to move this change along?

I feel there has to be a fundamental acceptance that it is ok for clang-format to alter code (something it already does with sorting of includes, namespace comments).

There were fairly strong opinions that clang-format isn't the best tool to do this (which actually I don't agree with, I think it is, as long as those capabilities are off by default and there is an acceptance they won't be perfect especially in the presence of macros due to lack of AST)

My only thought about building another tool would be if it was a drop in replacement for clang-format (tooling allows setting of a path), but it would need to inherit all of clang-format.

but to me, this just feels like extra grunt work just to work around why some community don't like it.

I guess a consensus is hard to come by, but I don't really know who owns the decision around the future direction of clang-format.

In D92257#2532071, @curdeius wrote:

LGTM. Could you please give us a link to the failing test in Polly? May be GitHub or buildbot.

The previous one broke a (format) test in polly. This lead me to change the one breaking behavior, no it is not breaking anymore.
A comment starting with } will only be indented if it is in a comment section which will get an indention. Test case is adapted.

I did add the commit here, but I can give you the hash explicitly: 9aa38a0615119a7a9f3bee33a2b8915c45f1cab9

Rebased and Ping.

Commited but forgot the Differential Link in the Message.

Do you need someone to push this? If so please post email and name to use for the commit.

I did not re-accept this, because of the script change. I'm okay with it, but I never looked really at the script. I think it should be changed afterwards to error or warn again, but not on this enum. Maybe one could annotate enums which don't have to be fully documented?

I can push that, but will wait a bit longer so the others have time to object.

Empty lines removed.

LGTM.

Now with assert.

Please expand the test case to different options like grouping. Otherwise looks good.

In D93986#2509079, @tinloaf wrote:

In D93986#2509045, @HazardyKnusperkeks wrote:

How is it going? I would like to add some new alignment options and it would be stupid not to do it on top of this.

Sorry, much to do work-wise currently. I'll get to the last details today and finish this!

How is it going? I would like to add some new alignment options and it would be stupid not to do it on top of this.

Is there a test with indentation within a namespace? If not maybe you should add one.

Found and fixed the case when no BasedOnStyle apart from InheritParentConfig is used.

I think that would have helped me also. I think it's not that easy, but honestly never checked.

In D93844#2504388, @HazardyKnusperkeks wrote:

In D93844#2504376, @MyDeveloperDay wrote:

What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.

Yes I do, but I will try to make it clearer in the tests.

Formatting corrected.

In D93844#2504376, @MyDeveloperDay wrote:

What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.

• Rebased
• Renamed to InhteritParentConfig
• Reworked to remove the recursion of getStyle
• Now supports the command line style options
• Test cases extended for the latter

• Rebased
• Fixed(?) the last UnitTest, please take a look @krasimir

Fix documentation.

Sorting corrected.

Not what I had in mind, but for me that's okay. I can not say anything to the change of the script though.

In D93986#2495561, @MyDeveloperDay wrote:

I think I would remove the code examples from the "AlignConsecutive style" to avoid confusion (that would be the first change)

then perhaps regenerate and update the documentation so we can then see, its seems most have a "code block", can the specific examples not go into that area?

And just have the enum explain its different configuration options?

/// \brief If ``true``, aligns consecutive C/C++ preprocessor macros.
///
/// This will align C/C++ preprocessor macros of consecutive lines.
/// Will result in formattings like
/// \code
///   #define SHORT_NAME       42
///   #define LONGER_NAME      0x007f
///   #define EVEN_LONGER_NAME (2)
///   #define foo(x)           (x * x)
///   #define bar(y, z)        (y + z)
/// \endcode
bool AlignConsecutiveMacros;

*ping*
And I don't have a better name.

I would add a test where you have a member before the first access modifier.
Also this is not exactly what they wanted in the bug, as far as I can see members of structs or classes with no access modifier at all should only be indented once.

In D92257#2497535, @MyDeveloperDay wrote:

My assumption is that you want to stick with the minimum and maximum is that correct?

Apart from that last naming issue looks good to me.

Apart from my inline comment and the pre merge check this is superb. (I will not accept it, until we have reached a conclusion for the documentation.)

In D93846#2488469, @thezbyg wrote:

Diff updated. Previous diff was generated after rebase, and Phabricator change preview did not show any unrelated changes, so I thought that everything is fine.

I think your base for the diff is wrong, there are many "added" things which are already in clang-format. Could you update the diff?