Yes, please could you commit it for me? Thanks!

Rebase patch on upstream HEAD

In D39363#948134, @alexfh wrote:

Sorry for the delay. I missed this revision somehow. Please add cfe-commits to the subscribers list so that others can chime in.

Factor !Type.isNull() && Type.isConstQualified() condition

In D30931#700690, @alexfh wrote:

I understand your use case, but this patch makes the check's behavior more confusing: having both "any case" and "ignore case" with subtle differences in behavior seems very misleading. The problem seems to be coming from the usage of CT_AnyCase to denote uninitialized style. Should we just remove NamingStyle::isSet and use llvm::Optional<NamingStyle> instead of NamingStyle where appropriate?

There's https://reviews.llvm.org/D27651 that will conflict with this one.

Awesome!

Yes you are right,

I'm trying to think of a scenario where *, && or & before tokens to be aligned would not indicate pointers or references, but as the alignment is only done for now on declarations and assignments, I can't find one.

In D21279#550565, @djasper wrote:

Are we talking completely past each other? I specifically think we should *NOT* combine NestingLevel and IndentLevel into one value. Not in ScopeLevel() and not anywhere else.

This patch might help clarifying the NestingLevel usage and can simplify the alignment computations in https://reviews.llvm.org/D21279

I had a little bit of look into the NestingLevel field. I understand that it only indicates the nesting level of the token inside the current AnnotatedLine, which could very well be the same as the nesting level of another token in the previous or next AnnotatedLine. Right now, it doesn't include the information on the nested level of the AnnotatedLine itself (this is in the Level field of the line itself).

In D21279#462578, @bmharper wrote:

2. friend functions: I don't really understand why the current behavior is what it is, but I think it's reasonable to argue that it actually improves readability by drawing attention to the fact these are friend functions, which ought to be quite rare in most code

Actually it looks like it works now... I'm not sure what I did when I had this misalignment. It would have been fine by me anyway.

Thanks! The operators are now correctly handled.

It looks like it doesn't like the operator[] either:

There's still cases where the nesting level is still not correctly handled: when using unbraced conditionals / loops. For example (sorry, silly example):

I think it's better to update the diff with full context (git diff -U999999) rather than attach the file, in order to have it integrated into phabricator diff view. Or use the arcanist command-line tool, which should do it for you.

Actually, the main issue that I see is the difficulty to build the plugin itself. The project does not build out-of-the-box on VS2013, and I had to change the assembly references by hand.

For information, I just tested the current diff under Visual Studio 2010 SP1 and everything looks fine as well.

• Patch rebased on trunk
• Small cleanups in the RunningDocumentTable usage
• Successfully tested on VS >= 2012

I don't have any compilation error with GCC 5.2.1. I tried again with Clang 3.8 and indeed there is this error:

I don't have commit access, if you don't mind landing it for me :)

Fix continue statements.

[clang-format] Code cleanup and variable naming in WhitespaceManager::AlignTokens

Updated the diff with a fix for the issue reported in http://lists.llvm.org/pipermail/cfe-dev/2015-November/046057.html

[clang-format] alignConsecutiveXXX: replace the buggy paren / braces counter with a better scope tracker.

Ping?

[clang-format] Replace TokenSequenceAligner with simple static functions.

[clang-format] Remove deleted methods declaration.

[clang-format] Alignment code factorization.

I've also added a fix for the other issue reported on the same bug. I could split the two reviews if necessary.

[clang-format] Count the number of braces and parens on the line instead of remembering only one.

Thanks, I don't have commit access.

Fix arcanist insisting on creating the diff against old revision...

• Add support of ColumnLimit restrition in AlignConsecutiveAssignments and AlignConsecutiveDeclarations.

Alright, thank you for your time!

Fix arcanist messup...

Address the latest comments, as well as useless code removal.

• Only the start of the token range is stored, so there is no need to care about the end of the range. Remove the code that was trying to match the end of the range (template, nested name).
• Add some spaces in test cases (destructors, templates) just to be sure that everything works as expected.
• Add a missing CHECK-FIXES in the test case on the abstract_class destructor.

Update the diff with the ProvideAutoLoad attribute.

Actually I believe it's better fixed by D13081.

I think the latest version also addresses D13090 in a better way. The check will now let clang-tidy decide whether warnings and fixes are displayed, whether it is in or from system/user header files or in main file, and will only worry about not emitting warnings and fixes if a macro is involved in the declaration or any usage.

Reworked the patches a little bit, addressing the comments and adding
better handling of checks in header files with corresponding unit test cases.

In D13079#253512, @alexfh wrote:

Please tell me, if you need me to commit the patch for you after you address the comments.

Yes please, I cannot commit it myself.

Added comments and reformated NamingCheckIdentifer.h with clang-format.

• Do not check for identifier names from system headers
• Check for SourceLocation validity before modifying them

This will also disable all warnings for declaration / usages outside of the main file.

Reformatting with clang-format

Update the diff with more context.

Update the diff with more context, thanks to arcanist.

Changed Usages to RawUsageLocs, as DenseSet storing the raw encoding of SourceLocation instead of a vector or SourceRange.

Well, the format on save setting is disabled by default, do you mean you had to enable it twice? Or you had it enabled without the C++ development tools, and after the installation you had to disable and enable it again?

Ping? Just to be sure it's not going to fall to oblivion. :)

Even better with the corresponding unit test fix.

Fix incorrect line removal in unit test.

@jbcoe Sorry for making this patch irrelevant but I managed to have the generic approach working in http://reviews.llvm.org/D13081, and I think it should cover this case as well. Thanks anyway for your work, and please keep contributing if you feel the check isn't perfect!

Remove remaining commented code.

I think that a more generic approach might be better, as discussed in http://reviews.llvm.org/D10933#225746. It should cover this particular case as well as other cases where class names are referenced.

The unit tests should work fine, now that D12369 has been merged.

I don't have credentials to commit it, anybody could do it for me if the diff is ok?

Here it is, with the typos in comments corrected, and rebased over the latest trunk.