LGTM. Well done!

I retract my objection. I forgot that from clang-format's perspective, #else blocks in separate functions have the same indent and nesting level, and thus the same "scope", so the alignment issues may persist there. You will see misalignment in #else blocks over an entire source file whether you keep or revert this fix (i.e. pick your poison.) I suppose based on precedent we keep the misalignments I wanted to fix since I introduced misalignments in other cases.

After reviewing the GitHub issue, it looks like the IndentAndNestingLevel is sticky over the entire scope for #else blocks, so the regression only occurs within #else blocks in the same scope. The bug I fixed affected alignment in all #else blocks, regardless of scope.

Do not limit columns and added nested test case.

Added context and use verifyFormat().

I am in favor of a new option, IncludeIsMainAllowBraces. It keeps things simple and does not cause any backwards-compatibility issues. The ${} variables are clever, but if "users unconcerned with formatting ... could get the current functionality by including a simple rule [that includes a variable]", I think it might be asking too much. Being able to write (and read) an include regex without referring to documentation seems ideal to me. You mentioned automatically adding variables in rules for backwards-compatibility, but I'd be a bit concerned about robustness and too much magic going on behind the scenes for users to understand if something goes wrong or has unexpected/unexplained results.

@MyDeveloperDay No. I was committing on behalf of someone else and I don't think they got around to fixing failing tests.

I am in favor of landing this and iterating.

LGTM.

I often:

Reverted this commit due to an unexpected test failure:

It's not more approval that is needed, it's just that someone with commit access (assuming you do not) needs to find the time to commit this. For what it's worth, I'm getting a patch rejection for the 4th block in lib/Format/Format.cpp. It seems the contents of LoadConfigFile need to be updated to reflect the most recent changes, so please rebase against master when you can.

Not that I am aware of. Whoever ends up doing the merge will likely run the necessary tests before committing. If you've run as many as you can, then hopefully all will be fine.

Digging through the history, it seems this behavior was explicitly desired: https://reviews.llvm.org/D19028. @mxbOctasic was the original author and perhaps could comment. Regardless, I think there should be a new option.

Assuming this passes all existing tests, LGTM.

LGTM

This feature is missing unit tests. Also, what is the reason for all the comment changes? I don't think the changed comment lines originally exceeded 80 characters.

Reopening in order to correct the accidentally swapped commits between this ticket and https://reviews.llvm.org/D70165.

Reopening in order to correct the accidentally swapped commits between this ticket and https://reviews.llvm.org/D69238.

Oops, it looks like I mixed up this ticket with https://reviews.llvm.org/D69238. Sorry about that. Would you like me to revert both commits and then re-commit with the correct links, etc.?

Yes, there was a failing unit test that had to be fixed. I reverted the first commit and then committed a version that fixed the failing test.

LGTM

LGTM

LGTM

LGTM

Never mind, I got it resolved and pushed. Sorry for the noise.

Thanks. Would you mind committing this on my behalf? It seems I wasn't migrated from SVN access to git access. It may take some time to sort out.

Sounds like you know what you're doing.

LGTM.

LGTM

Okay.

To me, whether or not the assertion was valid is irrelevant. That assertion was intentionally added, and its replacement with an if() statement materially changes the inputs and outputs of the function. I'm questioning whether the input of a "while" token within a preprocessor statement should output true or false. (Before, it didn't output anything; it errored.) Does this make sense? I'm just asking for clarification on this change.

I don't care for the command line switches that try to emulate compiler flags. I am also of the opinion that external scripts should be used for this functionality. git for Windows gives you a full set of bash utilities to utilize, so doing stuff like this on Windows is much easier now.

I agree with @djasper that this is outside the scope of clang-format. git for Windows gives you a full set of bash utilities to utilize, so doing stuff like this on Windows is much easier now.

I agree that a system in place that either enforces clang-formatting on commit or after the fact would be ideal. Otherwise, I don't see a need to have to approve these NFC commits.

LGTM

Thanks, please commit on my behalf.

This needs to be more thought out. At the very least it needs to cover more keywords like do, switch, try, and catch. There may be more. Consideration also needs to be made for class, functions, namespace, etc. Otherwise this is an experimental feature at best. There is also confusion with how it may interact with SpaceBeforeCpp11BracedList. The name SpacesAroundBraces is just too generic in that regard considering the fact that braces play multiple roles in C++.