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

[bmharper]

Summary

This patch fixes issues with AlignConsecutiveAssignments and
AlignConsecutiveDeclarations such as this:

Before:
  int fun1(int a);
  double fun2(int b);

  typedef pair<key_type, T> type1;
  typedef pair<X, Y> type2;

After:
  int    fun1(int a);
  double fun2(int b);

  typedef pair<key_type, T> type1;
  typedef pair<X, Y>        type2;

and...

Before:
  void fun(int x = 1) {
      int y      = 2;
  }

After:
  void fun(int x = 1) {
      int y = 2;
  }

The old alignment function was incapable of maintaining alignment whenever
the scope changed. To illustrate - in the first example mentioned, the
alignment of fun1 is lost by entering the nested scope of (int a).
This would cause the alignment to give up, and cause fun2 to start from a
blank slate. It would also cause false alignment, which is illustrated in
the second example above.

My primary motivator for this change is to have a list of function prototypes
line up.

This modification changes the alignment function so that it calls itself
recursively, at each change in scope depth. This allows it to maintain state
across different scope depths. A performance test against the current master
branch reveals a small (2.7%) speedup.

There are some new test cases which stress this functionality. In addition,
there were two historical test cases marked as "FIX ME", which now work as
intended.

In order to sense check, I have run this new implementation against the Clang
source code, with AlignConsecutiveAssignments:true and
AlignConsecutiveDeclarations:true. I then compared the old output with those
settings, vs the new output, with the same settings. All the changes that I
observed are explainable by the new logic, and IMO the formatted code looks
better with the new method.

[klimek]

Generally, please subscribe cfe-commits when sending patches via phab.
See http://llvm.org/docs/Phabricator.html

[kfunk]

In D21279#455923, @klimek wrote:

Generally, please subscribe cfe-commits when sending patches via phab.
See http://llvm.org/docs/Phabricator.html

You should set up a Herald rule then: https://secure.phabricator.com/book/phabricator/article/herald/ -- you can automatically subscribe users based on rules.

[djasper]

Could you upload a diff with full (i.e. the entire file as) context?

[bmharper]

As requested - full diff (of all three files)

AlignFullDiff.patch485 KBDownload

[berenm]

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.

Except from that, from a quick reading it looks good to me and I'm happy that someone fixed these cases, I see them from time to time actually but never had time to work on a proper fix.

[bmharper]

diff with full context

[berenm]

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

for (auto index = 0, e = 1000; index < e; ++index)
  int v = 0;
long double value = 1;

is aligned to

for (auto index = 0, e = 1000; index < e; ++index)
  int       v     = 0;
long double value = 1;

I'm not sure how to detect these unbraced scopes, maybe by also looking for different IndentLevel when computing the ScopeLevel?

[berenm]

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

struct test {
  long long int foo();
  int operator[](int a);
  double bar();

  long long int foo();
  int operator()(int a);
  double bar();
};

becomes:

struct test {
  long long int foo();
  int operator[](int a);
  double bar();

  long long int foo();
  int           operator()(int a);
  double        bar();
};

[bmharper]

Interesting. Working on it!

[bmharper]

Fix the recent two issues mentioned by Beren, ie the single-statement scopes (for loop without braces), and operator[] alignment.

Thanks for the IndentLevel suggestion - it is the best solution I can see.

[berenm]

Thanks! The operators are now correctly handled.

Another thing I've found is that constructors aren't aligned either with other declarations or together. Do you think it would be possible to treat them as functions as well?

Friend functions also aren't aligned with the other functions, but I'm not sure why or even if they should be. I believe most of the time friend functions are declared in a separate declaration "group" anyway.

struct A {
  explicit A(int);
  A();
  unsigned operator=(int a);
  long     bar(int a);
  friend void     foo();
  friend unsigned baz();
};

[bmharper]

I've taken some time to investigate those two issues, and these are my thoughts:

1. Constructor alignment: I think this is a good thing to do, but if isFunctionDeclarationName, and it's caller TokenAnnotator::calculateFormattingInformation are anything to go by, adding support for detection of constructors is going to be pretty hairy. I think I can see a way to do it, but it involves adding yet more complexity to TokenAnnotator::calculateFormattingInformation, and I'm not sure it's worth the effort. See TokenAnnotator.cpp for reference.

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

[berenm]

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.

Regarding constructors, your comment seems reasonable to me. The patch already improves the current state, so I think it's good like it is and further improvements could be added later on.

Thanks!

Ping @djasper for his review and eventual merge.

lib/Format/WhitespaceManager.cpp

89

Maybe we could spare the computation if we aren't going to align anything?

Is it better for clarity to always compute additional information? @djasper what's the Clang way to do?

[bmharper]

Hi Daniel,
Is there anything else that I need to do here?

Regards,
Ben

[bmharper]

Friendly PING.

Please let me know if there's anything else that I need to do here,
otherwise I'll keep quiet and expect a merge at some point?

[djasper]

Sorry.. Really busy at the moment, but will try to get this reviewed and submitted this week. If not, please ping again!

[bmharper]

kaPING!

[bmharper]

PING

lib/Format/WhitespaceManager.cpp

89

That's a good point. One certainly could elide that if alignment was turned off. I think so long as it was mentioned in the comments of the ScopeLevel member variable, it would be OK to do so. However, I'll also just defer this decision to @djasper.

[djasper]

Sorry :(... Review is easy enough.. Feel free to ping me more often in the future.

lib/Format/WhitespaceManager.cpp

89

Yeah, just avoid unnecessary work.

lib/Format/WhitespaceManager.h

158

NestingLevel does include braces, generally. However, there are two types:

• Braced initializers: These should just work as is.
• Braces that open blocks: Here, child lines are created and so the tokens within a block restart from NestingLevel 0. However, taking that NestingLevel in combination with the Level of the AnnotatedLine should work.

I think reusing that is highly preferable over implementing yet another parentheses counting.

[bmharper]

Fixed the two issues pointed out by djasper in his most recent comments:

1. Only calculate ScopeLevel when necessary.
2. Instead of calculating ScopeLevel by inspecting {[(< and their closing pairs, calculate it by combining IndentLevel and Nesting Level. It's not quite as simple as just making ScopeLevel = IndentLevel + NestingLevel, but at least we don't have yet another function computing scope depth from scratch.

[djasper]

I think instead of doing some complex computation with LineLevel and NestingLevel, it might be better to just leave them as the pair and compare them as a pair. The LineLevel should probably always trump the NestingLevel. So, I'd try to just defined ScopeLevel as a pair<int, int>, put both the LineLevel and the NestingLevel in it and use that for the comparisons. I might be overlooking something, though.

[bmharper]

The reason one has to precompute ScopeLevel is because IndentLevel is not actually meaningful on each token. It's only meaningful for the first token on the line (the remaining tokens on the line have IndentLevel = 0). So if you look at the implementation of calculateScopeLevel(), you'll see that the function "remembers" the most recent meaningful IndentLevel, and copies that into ScopeLevel for all subsequent tokens on the same line.
Now, one could argue that IndentLevel ought to be the same for all tokens on a line, but that seems to me like a much bigger change, that would propagate into many other parts of the code.

[djasper]

I think the IndentLevel in WhitespaceManager (and the nested Change) is a horrible mess and should be cleaned up. It gets set either to 0 or to the "Level" of the AnnotatedLine. To me only the latter makes sense as the line defines the indent level. Everything else, including recomputing this and introducing a ScopeLevel makes the current situation worse. I can try to do this in advance of this patch, if you prefer.

[bmharper]

I'll see what happens if I make IndentLevel the same for all tokens on a line. If not too much is broken by that, I should be able to handle it.

[bmharper]

I have added an initial phase which propagates IndentLevel from the first token on a line, to all of the tokens on that line. This allows us to get rid of the ScopeLevel state. However, I have retained the name "ScopeLevel", and made it a member function of Change. I think this is better than putting IndentLevel and NestingLevel inside an std::pair, because by retaining the words IndentLevel and NestingLevel, it's easy to navigate the rest of the source code and discover where those values come from. Additionally, the function ScopeLevel() needs to execute one tiny, but vital piece of logic, in order to be useful for alignment purposes. One cannot simply add IndentLevel and NestingLevel together. This is explained in detail inside the body of the ScopeLevel() function.

[bmharper]

PING!
My previous commit hopefully addressed the issue with the sprawl of IndentLevel + ScopeLevel

[djasper]

lib/Format/WhitespaceManager.cpp

49

What I don't understand is why you have to combine NestingLevel and IndentLevel at all. To me it feels wrong to add them no matter what (with and without your extra bit of logic).

IMO, for alignment, we should ensure that both NestingLevel *and* IndentLevel are the same, not just the the sum of the two is the same. That's why I was suggesting putting them into a pair and just comparing the pair. But I might be missing something very obvious.

[nikola]

lib/Format/WhitespaceManager.cpp

350–351

Comment is out of date, it's still talking about scope depth.

[bmharper]

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

[berenm]

Hello,

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).

I'm not sure if the value of IndentLevel comes directly from the AnnotatedLine's Level, but I believe that combining them would give an equivalent of a "global" nesting level of the token.

In order to be cleaner, I think it could be done in the AnnotatingParser class, that already fills the NestingLevel field. I wrote a patch that demonstrates this here: https://reviews.llvm.org/D24859. All the unit tests are passing, although I'm not 100% sure about all the implications this change has. With this patch, I believed NestingLevel can be used directly to determine begin and end of alignment sequences, without requiring the ScopeLevel helper function.

[djasper]

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.

[berenm]

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.

Ok, I probably misunderstood the situation, sorry.

[bmharper]

This revision merges NestingLevel and IndentLevel into an std::pair, as suggested by @djasper.
IMO this makes the code slightly harder to read, because you lose the unique variable names "IndentLevel" and "NestingLevel". Those are now replaced by .first and .second, respectively. In addition, NestingLevel is no longer equal to the original NestingLevel that gets passed in, because it needs to be tweaked slightly to better work with the recursive technique we use for alignment scope.
However, the approach presented here certainly does work, and it's actually not too much of a code change from the previous patch.

[bmharper]

ping!

[djasper]

So sorry. Seems I forgot to hit "Submit" :(.

If you don't like the ".first" and ".second" of the pair, you could introduce a struct for it and overload operator<. Might actually be more readable.

lib/Format/WhitespaceManager.cpp

62–63

nit: Move these to the previous line. clang-format won't do that, because of the comment, but that's actually irrelevant here.

198

Make this (and maybe a few others) more concrete. Don't write "some special tokens", write what they actually are.

202

If the example isn't too long, writing the source code in the comment seems better than referencing the test.

206

I don't see why this would be necessary. If I remove it, all tests do still pass.

213

Also, this comment seems wrong? The "int x = 1;" actually starts a new (child) line. If that has the same nesting level, that seems like a bug we need to fix.

378

Either do:

EndOfSequence = StoppedAt;

or just remove StoppedAt and use i.

unittests/Format/FormatTest.cpp

7660

Can you add a test case where there is a line wrap after the "("?

[bmharper]

Sorry for the incredibly long turnaround time on this one - I don't have any legitimate excuse, and I know it just makes reviewing it harder.

Anyway - I have sorted out all the issues mentioned in the last review. One thing that you suggested was to go ahead and replace the std::pair for NestingAndIndentLevel, which I did write, but I feel like it adds quite a bit of bloat, because you need two constructors, operator==, operator!=, operator<, operator>, so I'm not convinced that it's worth it in the end. If you still think it is, then I'll go ahead and add it.

Regards,
Ben

[berenm]

Pinging @djasper

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

[enyquist]

Hey bmharper :) I've got a review open that conflicts with this one, just having a look to see what I'll need to refactor
(https://reviews.llvm.org/D28462).

In fact, I have a question-- the conflict is specifically in WhitespaceManager.cpp. Since I needed to detect PP macros containing changes in scope depth (code blocks surrounded by curly braces, macro parameter lists, etc), I was having the same problem as you-- AlignTokens was bailing out whenever the scope depth changed.

In my case, I just added a new parameter to AlignTokens, MaxNestingLevelIncrease, indicating how much the level can increase before we stop alignment, making the allowable scope-depth configurable. For example, calling AlignTokens with this value set to 2 will cause alignment to continue up until we increase scope by two levels.

Now, my question- from what I can tell of your changes, it looks like my code can actually be simpler when this gets merged. The state of AlignTokens will be maintained across changing scope depths, and I won't need to modify AlignTokens so that it can survive something like "#define foo(x) ((x * 2) + 2)". Is this correct?

[bmharper]

Hi @enyquist,
I'd like to guess that this patch will solve your problem, but I'm not intimate enough with this code to give you a definitive answer. I hope we can get this merged soon, so that you can just run it and see.

Ben

[enyquist]

Well, your patch is here for me to try, and it looks like it's been accepted. So I guess I should just pull my finger out and try it :)
Thanks for your response-- I'll let you know if I come across any issues.

[bmharper]

Pinging @djasper. Any chance we can get this merged?

[djasper]

lib/Format/WhitespaceManager.cpp

215

Merge the two ifs into a single one?

324

I'd probably move the second condition into an early exit inside the loop:

if (Changes[i].NestingAndIndentLevel >= NestingAndIndentLevel)
  break;

399

Use a comment to describe the literal, i.e.:

/*StartAt=*/0

lib/Format/WhitespaceManager.h

122

This comment seems outdated.

202

This function should be a local function in the .cpp file. Also, can you describe in more detail what this does, i.e. what kind of declarations are covered here? Also, it is a bit confusing to have a function and a member of WhitespaceManager::Change with the exact same name.

[djasper]

I apologize in advance if this causes merge conflicts with r293616. However, I do hope that that actually makes this patch easier.

[bmharper]

Thanks - the merge conflicts don't look too bad. I should have it cleaned up by tomorrow.

[daphnediane]

In D21279#663670, @bmharper wrote:

Thanks - the merge conflicts don't look too bad. I should have it cleaned up by tomorrow.

Have you had a chance to do the merge yet? If not I have a merged version which passes the tests that I could post here if you want.

[bmharper]

Hi @djasper,
I've made the latest bunch of review changes, and rebased onto master. I didn't check the numbers, but I think the code is slightly smaller after the rebase.

Regards,
Ben

[bmharper]

Fixed a stale comment

[bmharper]

Thanks @daphnediane. I only read your comment after merging, but that would have been helpful.

[djasper]

lib/Format/WhitespaceManager.cpp

188

I don't think you need this struct now. Just use the FormatToken itself, it should have all of this information.

193

This is no longer true. IndentLevel should be set correctly for every token.

269

nit: s/exits/returns/

(or maybe even "returns the current position")

340–344

The "!=" is a bit confusing here. ">" would do the same thing, right (because "<" is already handled above)?

lib/Format/WhitespaceManager.h

130

I think you can get rid of this field now and just directly use the values stored in the tokens.

[daphnediane]

In D21279#667471, @bmharper wrote:

Thanks @daphnediane. I only read your comment after merging, but that would have been helpful.

If you still want my diff let me know as it is slightly different from yours. No longer has NestingAndIndent level as a data member of Changes ( but has a inline method that gets the values though not 100% sure that is needed as I experimented with a version without it), no longer needs propagateIndentLevels, etc.

[bmharper]

This looks a lot better IMO. Thanks @daphnediane - you should recognize the code ;)
The special closing paren logic inside of nestingAndIndentLevel() is indeed redundant now.

[djasper]

This looks very nice now :-D. Thanks for working on this!!

lib/Format/WhitespaceManager.cpp

196

Maybe add: "It contains indices of the first token on each scope."

[bmharper]

small comment tweak

[bmharper]

Thanks for all the code review work! I'm impressed by the quality standard maintained here.

[daphnediane]

Rebuilt with the latest patch and got one compile error. See line comment worked okay after fixing it.

lib/Format/WhitespaceManager.cpp

215

These ifs can get merged again, when you merged my changes in it was based on a version before you merged them.

lib/Format/WhitespaceManager.h

157

Extra WhitespaceManager::Change:: prefix here

[bmharper]

Fixed two small issues raised by @daphnediane

[bmharper]

Hi @djasper,
This is the first patch I've contributed here, so I'm not familiar with the whole process. I assume this code is ready to land? When exactly does it get merged into master, and is there something else that I still need to do to make that happen?

Thanks,
Ben

[daphnediane]

ping

[nikola]

Commit r298574, thanks for woking on this folks!

lib/Format/WhitespaceManager.h

struct Change {
     // Changes might be in the middle of a token, so we cannot just keep the
     // FormatToken around to query its information.
     SourceRange OriginalWhitespaceRange;
     unsigned StartOfTokenColumn;
     unsigned NewlinesBefore;
     std::string PreviousLinePostfix;
     std::string CurrentLinePrefix;
     bool ContinuesPPDirective;
 
     // The number of spaces in front of the token or broken part of the token.
     // This will be adapted when aligning tokens.
     // Can be negative to retain information about the initial relative offset
     // of the lines in a block comment. This is used when aligning trailing
     // comments. Uncompensated negative offset is truncated to 0.
     int Spaces;
 
     // If this change is inside of a token but not at the start of the token or
     // directly after a newline.
     bool IsInsideToken;
 
     // \c IsTrailingComment, \c TokenLength, \c PreviousEndOfTokenColumn and
     // \c EscapedNewlineColumn will be calculated in
     // \c calculateLineBreakInformation.
     bool IsTrailingComment;
     unsigned TokenLength;
     unsigned PreviousEndOfTokenColumn;
     unsigned EscapedNewlineColumn;
 
     // These fields are used to retain correct relative line indentation in a
     // block comment when aligning trailing comments.
     //
     // If this Change represents a continuation of a block comment,
     // \c StartOfBlockComment is pointer to the first Change in the block
     // comment. \c IndentationOffset is a relative column offset to this
     // change, so that the correct column can be reconstructed at the end of
     // the alignment process.
     const Change *StartOfBlockComment;
     int IndentationOffset;
+
+    // A combination of nesting level and indent level, which are used in
+    // tandem to compute lexical scope, for the purposes of deciding
+    // when to stop consecutive alignment runs.
+    std::pair<unsigned, unsigned>
+    nestingAndIndentLevel() const {
+      return std::make_pair(Tok->NestingLevel, Tok->IndentLevel);
+    }
   };
 
 private:
   /// \brief Calculate \c IsTrailingComment, \c TokenLength for the last tokens
   /// or token parts in a line and \c PreviousEndOfTokenColumn and
   /// \c EscapedNewlineColumn for the first tokens or token parts in a line.
   void calculateLineBreakInformation();
 
private:
   void appendNewlineText(std::string &Text, unsigned Newlines,
                          unsigned PreviousEndOfTokenColumn,
                          unsigned EscapedNewlineColumn);
   void appendIndentText(std::string &Text, unsigned IndentLevel,
                         unsigned Spaces, unsigned WhitespaceStartColumn);
 
   SmallVector<Change, 16> Changes;
   const SourceManager &SourceMgr;
   tooling::Replacements Replaces;
   const FormatStyle &Style;
   bool UseCRLF;
 };
 
 } // namespace format
 } // namespace clang
 
 #endif

lib/Format/WhitespaceManager.cpp

: Tok(&Tok), CreateReplacement(CreateReplacement),
       ContinuesPPDirective(ContinuesPPDirective), Spaces(Spaces),
       IsInsideToken(IsInsideToken), IsTrailingComment(false), TokenLength(0),
       PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
       StartOfBlockComment(nullptr), IndentationOffset(0) {}
 
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
                                           unsigned Spaces,
                                           unsigned StartOfTokenColumn,
                                           bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
-  Changes.push_back(Change(Tok, /*CreateReplacement=*/true,
-                           Tok.WhitespaceRange, Spaces, StartOfTokenColumn,
-                           Newlines, "", "", InPPDirective && !Tok.IsFirst,
+  Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
+                           Spaces, StartOfTokenColumn, Newlines, "", "",
+                           InPPDirective && !Tok.IsFirst,
                            /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::addUntouchableToken(const FormatToken &Tok,
                                             bool InPPDirective) {
   if (Tok.Finalized)
     return;
   Changes.push_back(Change(Tok, /*CreateReplacement=*/false,
                            Tok.WhitespaceRange, /*Spaces=*/0,
                            Tok.OriginalColumn, Tok.NewlinesBefore, "", "",
                            InPPDirective && !Tok.IsFirst,
                            /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::replaceWhitespaceInToken(
     const FormatToken &Tok, unsigned Offset, unsigned ReplaceChars,
Changes.push_back(
              InPPDirective && !Tok.IsFirst, /*IsInsideToken=*/true));
 }
 
 const tooling::Replacements &WhitespaceManager::generateReplacements() {
   if (Changes.empty())
     return Replaces;
 
   std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr));
   calculateLineBreakInformation();
   alignConsecutiveDeclarations();
   alignConsecutiveAssignments();
   alignTrailingComments();
   alignEscapedNewlines();
   generateChanges();
 
   return Replaces;
 }
if (Change.Tok->is(tok::comment)) {
               Change.StartOfBlockComment->StartOfTokenColumn;
       }
     } else {
       LastBlockComment = nullptr;
     }
   }
 }
 
 // Align a single sequence of tokens, see AlignTokens below.
 template <typename F>
 static void
 AlignTokenSequence(unsigned Start, unsigned End, unsigned Column, F &&Matches,
                    SmallVector<WhitespaceManager::Change, 16> &Changes) {
   bool FoundMatchOnLine = false;
   int Shift = 0;
+
+  // ScopeStack keeps track of the current scope depth. It contains indices of
+  // the first token on each scope.
+  // We only run the "Matches" function on tokens from the outer-most scope.
+  // However, we do need to pay special attention to one class of tokens
+  // that are not in the outer-most scope, and that is function parameters
+  // which are split across multiple lines, as illustrated by this example:
+  //   double a(int x);
+  //   int    b(int  y,
+  //          double z);
+  // In the above example, we need to take special care to ensure that
+  // 'double z' is indented along with it's owning function 'b'.
+  SmallVector<unsigned, 16> ScopeStack;
+
   for (unsigned i = Start; i != End; ++i) {
-    if (Changes[i].NewlinesBefore > 0) {
-      FoundMatchOnLine = false;
+    if (ScopeStack.size() != 0 &&
+        Changes[i].nestingAndIndentLevel() <
+            Changes[ScopeStack.back()].nestingAndIndentLevel())
+      ScopeStack.pop_back();
+
+    if (i != Start && Changes[i].nestingAndIndentLevel() >
+                          Changes[i - 1].nestingAndIndentLevel())
+      ScopeStack.push_back(i);
+
+    bool InsideNestedScope = ScopeStack.size() != 0;
+
+    if (Changes[i].NewlinesBefore > 0 && !InsideNestedScope) {
       Shift = 0;
+      FoundMatchOnLine = false;
     }
 
     // If this is the first matching token to be aligned, remember by how many
     // spaces it has to be shifted, so the rest of the changes on the line are
     // shifted by the same amount
-    if (!FoundMatchOnLine && Matches(Changes[i])) {
+    if (!FoundMatchOnLine && !InsideNestedScope && Matches(Changes[i])) {
       FoundMatchOnLine = true;
       Shift = Column - Changes[i].StartOfTokenColumn;
       Changes[i].Spaces += Shift;
     }
 
+    // This is for function parameters that are split across multiple lines,
+    // as mentioned in the ScopeStack comment.
+    if (InsideNestedScope && Changes[i].NewlinesBefore > 0) {
+      unsigned ScopeStart = ScopeStack.back();
+      if (Changes[ScopeStart - 1].Tok->is(TT_FunctionDeclarationName) ||
+          (ScopeStart > Start + 1 &&
+           Changes[ScopeStart - 2].Tok->is(TT_FunctionDeclarationName)))
+        Changes[i].Spaces += Shift;
+    }
+
     assert(Shift >= 0);
     Changes[i].StartOfTokenColumn += Shift;
     if (i + 1 != Changes.size())
       Changes[i + 1].PreviousEndOfTokenColumn += Shift;
   }
 }
 
-// Walk through all of the changes and find sequences of matching tokens to
-// align. To do so, keep track of the lines and whether or not a matching token
-// was found on a line. If a matching token is found, extend the current
-// sequence. If the current line cannot be part of a sequence, e.g. because
-// there is an empty line before it or it contains only non-matching tokens,
-// finalize the previous sequence.
+// Walk through a subset of the changes, starting at StartAt, and find
+// sequences of matching tokens to align. To do so, keep track of the lines and
+// whether or not a matching token was found on a line. If a matching token is
+// found, extend the current sequence. If the current line cannot be part of a
+// sequence, e.g. because there is an empty line before it or it contains only
+// non-matching tokens, finalize the previous sequence.
+// The value returned is the token on which we stopped, either because we
+// exhausted all items inside Changes, or because we hit a scope level higher
+// than our initial scope.
+// This function is recursive. Each invocation processes only the scope level
+// equal to the initial level, which is the level of Changes[StartAt].
+// If we encounter a scope level greater than the initial level, then we call
+// ourselves recursively, thereby avoiding the pollution of the current state
+// with the alignment requirements of the nested sub-level. This recursive
+// behavior is necessary for aligning function prototypes that have one or more
+// arguments.
+// If this function encounters a scope level less than the initial level,
+// it returns the current position.
+// There is a non-obvious subtlety in the recursive behavior: Even though we
+// defer processing of nested levels to recursive invocations of this
+// function, when it comes time to align a sequence of tokens, we run the
+// alignment on the entire sequence, including the nested levels.
+// When doing so, most of the nested tokens are skipped, because their
+// alignment was already handled by the recursive invocations of this function.
+// However, the special exception is that we do NOT skip function parameters
+// that are split across multiple lines. See the test case in FormatTest.cpp
+// that mentions "split function parameter alignment" for an example of this.
 template <typename F>
-static void AlignTokens(const FormatStyle &Style, F &&Matches,
-                        SmallVector<WhitespaceManager::Change, 16> &Changes) {
+static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
+                            SmallVector<WhitespaceManager::Change, 16> &Changes,
+                            unsigned StartAt) {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
 
   // Line number of the start and the end of the current token sequence.
   unsigned StartOfSequence = 0;
   unsigned EndOfSequence = 0;
 
-  // Keep track of the nesting level of matching tokens, i.e. the number of
-  // surrounding (), [], or {}. We will only align a sequence of matching
-  // token that share the same scope depth.
-  //
-  // FIXME: This could use FormatToken::NestingLevel information, but there is
-  // an outstanding issue wrt the brace scopes.
-  unsigned NestingLevelOfLastMatch = 0;
-  unsigned NestingLevel = 0;
+  // Measure the scope level (i.e. depth of (), [], {}) of the first token, and
+  // abort when we hit any token in a higher scope than the starting one.
+  auto NestingAndIndentLevel = StartAt < Changes.size()
+                                   ? Changes[StartAt].nestingAndIndentLevel()
+                                   : std::pair<unsigned, unsigned>(0, 0);
 
   // Keep track of the number of commas before the matching tokens, we will only
   // align a sequence of matching tokens if they are preceded by the same number
   // of commas.
   unsigned CommasBeforeLastMatch = 0;
   unsigned CommasBeforeMatch = 0;
 
   // Whether a matching token has been found on the current line.
if (StartOfSequence > 0 && StartOfSequence < EndOfSequence)
       AlignTokenSequence(StartOfSequence, EndOfSequence, MinColumn, Matches,
                          Changes);
     MinColumn = 0;
     MaxColumn = UINT_MAX;
     StartOfSequence = 0;
     EndOfSequence = 0;
   };
 
-  for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
+  unsigned i = StartAt;
+  for (unsigned e = Changes.size(); i != e; ++i) {
+    if (Changes[i].nestingAndIndentLevel() < NestingAndIndentLevel)
+      break;
+
     if (Changes[i].NewlinesBefore != 0) {
       CommasBeforeMatch = 0;
       EndOfSequence = i;
       // If there is a blank line, or if the last line didn't contain any
       // matching token, the sequence ends here.
       if (Changes[i].NewlinesBefore > 1 || !FoundMatchOnLine)
         AlignCurrentSequence();
 
       FoundMatchOnLine = false;
     }
 
     if (Changes[i].Tok->is(tok::comma)) {
       ++CommasBeforeMatch;
-    } else if (Changes[i].Tok->isOneOf(tok::r_brace, tok::r_paren,
-                                       tok::r_square)) {
-      --NestingLevel;
-    } else if (Changes[i].Tok->isOneOf(tok::l_brace, tok::l_paren,
-                                       tok::l_square)) {
-      // We want sequences to skip over child scopes if possible, but not the
-      // other way around.
-      NestingLevelOfLastMatch = std::min(NestingLevelOfLastMatch, NestingLevel);
-      ++NestingLevel;
+    } else if (Changes[i].nestingAndIndentLevel() > NestingAndIndentLevel) {
+      // Call AlignTokens recursively, skipping over this scope block.
+      unsigned StoppedAt = AlignTokens(Style, Matches, Changes, i);
+      i = StoppedAt - 1;
+      continue;
     }
 
     if (!Matches(Changes[i]))
       continue;
 
     // If there is more than one matching token per line, or if the number of
-    // preceding commas, or the scope depth, do not match anymore, end the
-    // sequence.
-    if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch ||
-        NestingLevel != NestingLevelOfLastMatch)
+    // preceding commas, do not match anymore, end the sequence.
+    if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch)
       AlignCurrentSequence();
 
     CommasBeforeLastMatch = CommasBeforeMatch;
-    NestingLevelOfLastMatch = NestingLevel;
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
       StartOfSequence = i;
 
     unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
     int LineLengthAfter = -Changes[i].Spaces;
     for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j)
       LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
     // If we are restricted by the maximum column width, end the sequence.
     if (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn ||
         CommasBeforeLastMatch != CommasBeforeMatch) {
       AlignCurrentSequence();
       StartOfSequence = i;
     }
 
     MinColumn = std::max(MinColumn, ChangeMinColumn);
     MaxColumn = std::min(MaxColumn, ChangeMaxColumn);
   }
 
-  EndOfSequence = Changes.size();
+  EndOfSequence = i;
   AlignCurrentSequence();
+  return i;
 }
 
 void WhitespaceManager::alignConsecutiveAssignments() {
   if (!Style.AlignConsecutiveAssignments)
     return;
 
   AlignTokens(Style,
               [&](const Change &C) {
                 // Do not align on equal signs that are first on a line.
                 if (C.NewlinesBefore > 0)
                   return false;
 
                 // Do not align on equal signs that are last on a line.
                 if (&C != &Changes.back() && (&C + 1)->NewlinesBefore > 0)
                   return false;
 
                 return C.Tok->is(tok::equal);
               },
-              Changes);
+              Changes, /*StartAt=*/0);
 }
 
 void WhitespaceManager::alignConsecutiveDeclarations() {
   if (!Style.AlignConsecutiveDeclarations)
     return;
 
   // FIXME: Currently we don't handle properly the PointerAlignment: Right
   // The * and & are not aligned and are left dangling. Something has to be done
   // about it, but it raises the question of alignment of code like:
   //   const char* const* v1;
   //   float const* v2;
   //   SomeVeryLongType const& v3;
-
   AlignTokens(Style,
               [](Change const &C) {
-                return C.Tok->isOneOf(TT_StartOfName,
-                                      TT_FunctionDeclarationName);
+                // tok::kw_operator is necessary for aligning operator overload
+                // definitions.
+                return C.Tok->is(TT_StartOfName) ||
+                       C.Tok->is(TT_FunctionDeclarationName) ||
+                       C.Tok->is(tok::kw_operator);
               },
-              Changes);
+              Changes, /*StartAt=*/0);
 }
 
 void WhitespaceManager::alignTrailingComments() {
   unsigned MinColumn = 0;
   unsigned MaxColumn = UINT_MAX;
   unsigned StartOfSequence = 0;
   bool BreakBeforeNext = false;
   unsigned Newlines = 0;

unittests/Format/FormatTest.cpp

verifyFormat("auto lambda = []() {\n"
                "int i  = 0;\n"
                "auto v = type{\n"
                "    i = 1,   //\n"
                "    (i = 2), //\n"
                "    i = 3    //\n"
                "};",
                Alignment);
 
-  // FIXME: Should align all three assignments
   verifyFormat(
       "int i      = 1;\n"
       "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n"
       "                          loooooooooooooooooooooongParameterB);\n"
-      "int j = 2;",
+      "int j      = 2;",
       Alignment);
 
   verifyFormat("template <typename T, typename T_0 = very_long_type_name_0,\n"
                "          typename B   = very_long_type_name_1,\n"
                "          typename T_2 = very_long_type_name_2>\n"
                "auto foo() {}\n",
                Alignment);
   verifyFormat("int a, b = 1;\n"
                "int c  = 2;\n"
                "int dd = 3;\n",
                Alignment);
   verifyFormat("int aa       = ((1 > 2) ? 3 : 4);\n"
                "float b[1][] = {{3.f}};\n",
                Alignment);
+  verifyFormat("for (int i = 0; i < 1; i++)\n"
+               "  int x = 1;\n",
+               Alignment);
+  verifyFormat("for (i = 0; i < 1; i++)\n"
+               "  x = 1;\n"
+               "y = 1;\n",
+               Alignment);
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveDeclarations = false;
   verifyFormat("float const a = 5;\n"
                "int oneTwoThree = 123;",
                Alignment);
EXPECT_EQ("float a = 5;\n"
             "unsigned oneTwoThree = 123;\n"
             "int      oneTwo = 12;",
             format("float    a = 5;\n"
                    "int one = 1;\n"
                    "\n"
                    "unsigned oneTwoThree = 123;\n"
                    "int oneTwo = 12;",
                    Alignment));
+  // Function prototype alignment
+  verifyFormat("int    a();\n"
+               "double b();",
+               Alignment);
+  verifyFormat("int    a(int x);\n"
+               "double b();",
+               Alignment);
+  unsigned OldColumnLimit = Alignment.ColumnLimit;
+  // We need to set ColumnLimit to zero, in order to stress nested alignments,
+  // otherwise the function parameters will be re-flowed onto a single line.
+  Alignment.ColumnLimit = 0;
+  EXPECT_EQ("int    a(int   x,\n"
+            "         float y);\n"
+            "double b(int    x,\n"
+            "         double y);",
+            format("int a(int x,\n"
+                   " float y);\n"
+                   "double b(int x,\n"
+                   " double y);",
+                   Alignment));
+  // This ensures that function parameters of function declarations are
+  // correctly indented when their owning functions are indented.
+  // The failure case here is for 'double y' to not be indented enough.
+  EXPECT_EQ("double a(int x);\n"
+            "int    b(int    y,\n"
+            "         double z);",
+            format("double a(int x);\n"
+                   "int b(int y,\n"
+                   " double z);",
+                   Alignment));
+  // Set ColumnLimit low so that we induce wrapping immediately after
+  // the function name and opening paren.
+  Alignment.ColumnLimit = 13;
+  verifyFormat("int function(\n"
+               "    int  x,\n"
+               "    bool y);",
+               Alignment);
+  Alignment.ColumnLimit = OldColumnLimit;
+  // Ensure function pointers don't screw up recursive alignment
+  verifyFormat("int    a(int x, void (*fp)(int y));\n"
+               "double b();",
+               Alignment);
   Alignment.AlignConsecutiveAssignments = true;
+  // Ensure recursive alignment is broken by function braces, so that the
+  // "a = 1" does not align with subsequent assignments inside the function
+  // body.
+  verifyFormat("int func(int a = 1) {\n"
+               "  int b  = 2;\n"
+               "  int cc = 3;\n"
+               "}",
+               Alignment);
   verifyFormat("float      something = 2000;\n"
                "double     another   = 911;\n"
                "int        i = 1, j = 10;\n"
                "const int *oneMore = 1;\n"
                "unsigned   i       = 2;",
                Alignment);
   verifyFormat("int      oneTwoThree = {0}; // comment\n"
                "unsigned oneTwo      = 0;   // comment",
                Alignment);
+  // Make sure that scope is correctly tracked, in the absence of braces
+  verifyFormat("for (int i = 0; i < n; i++)\n"
+               "  j = i;\n"
+               "double x = 1;\n",
+               Alignment);
+  verifyFormat("if (int i = 0)\n"
+               "  j = i;\n"
+               "double x = 1;\n",
+               Alignment);
+  // Ensure operator[] and operator() are comprehended
+  verifyFormat("struct test {\n"
+               "  long long int foo();\n"
+               "  int           operator[](int a);\n"
+               "  double        bar();\n"
+               "};\n",
+               Alignment);
+  verifyFormat("struct test {\n"
+               "  long long int foo();\n"
+               "  int           operator()(int a);\n"
+               "  double        bar();\n"
+               "};\n",
+               Alignment);
   EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
             "  int const i   = 1;\n"
             "  int *     j   = 2;\n"
             "  int       big = 10000;\n"
             "\n"
             "  unsigned oneTwoThree = 123;\n"
             "  int      oneTwo      = 12;\n"
             "  method();\n"
verifyFormat("auto lambda = []() {\n"
                "auto  v  = type{\n"
                "    i = 1,   //\n"
                "    (i = 2), //\n"
                "    i = 3    //\n"
                "};",
                Alignment);
   Alignment.AlignConsecutiveAssignments = false;
 
-  // FIXME: Should align all three declarations
   verifyFormat(
       "int      i = 1;\n"
       "SomeType a = SomeFunction(looooooooooooooooooooooongParameterA,\n"
       "                          loooooooooooooooooooooongParameterB);\n"
-      "int j = 2;",
+      "int      j = 2;",
       Alignment);
 
   // Test interactions with ColumnLimit and AlignConsecutiveAssignments:
   // We expect declarations and assignments to align, as long as it doesn't
-  // exceed the column limit, starting a new alignemnt sequence whenever it
+  // exceed the column limit, starting a new alignment sequence whenever it
   // happens.
   Alignment.AlignConsecutiveAssignments = true;
   Alignment.ColumnLimit = 30;
   verifyFormat("float    ii              = 1;\n"
                "unsigned j               = 2;\n"
                "int someVerylongVariable = 1;\n"
                "AnotherLongType  ll = 123456;\n"
                "VeryVeryLongType k  = 2;\n"