[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Ben Harper via cfe-commits]

bmharper added a comment.

So.. I finally got some time to look at this again:

Quick Recap - IndentLevel and NestingLevel are now stored separately inside WhitespaceManager::Change. I've added a function ScopeLevel() which combines them with a bit of logic, and returns a number that can be used for alignment scope detection purposes.
Now the reason why I don't want to combine IndentLevel and NestingLevel into one value:

IndentLevel is used by a function called WhitespaceManager::appendIndentText. I don't understand exactly what this function is doing, and despite some attempts, I haven't managed to craft an input which gets it to run down the code paths I'm interested in. Now as far as I can tell from the experiments I've been able to come up with, it's OK to combine IndentLevel and NestingLevel into a single number, because it has no observable effect on appendIndentText. HOWEVER, just because I can't reproduce the conditions necessary for that code to run, doesn't mean there isn't some body of code out there that does stress those code paths. It seems like a reasonable approach to me to maintain the separate variables IndentLevel and NestingLevel, primarily because those keywords are searchable in the rest of the code, and it's easy to trace their lineage back to the places where they're generated. If we were to combine them, and appendIndentText happens to be broken by this change, then it's going to be very confusing for the next guy to come and figure out why this is so. At present, IndentLevel means what it says, and so does NestingLevel, and I believe, so does ScopeLevel, so I think it's a better idea to keep them separate.

Ben


https://reviews.llvm.org/D21279