[clang-format] Fixup #include guard indents after parseFile()

[mzeren-vmw]

Summary

When a preprocessor indent closes after the last line of normal code we do not
correctly fixup include guard indents. For example:

#ifndef HEADER_H
#define HEADER_H
#if 1
int i;
#  define A 0
#endif
#endif

incorrectly reformats to:

#ifndef HEADER_H
#define HEADER_H
#if 1
int i;
#    define A 0
#  endif
#endif

To resolve this issue we must fixup levels after parseFile(). Delaying
the fixup introduces a new state, so consolidate include guard search
state into an enum.

[mzeren-vmw]

rebase, ping.

[krasimir]

Looks good! Just some comments for the include guard states would be helpful.

lib/Format/UnwrappedLineParser.h

259 ↗

(On Diff #132320)

Please put a short comment explaining each of these states.

[mzeren-vmw]

lib/Format/UnwrappedLineParser.cpp

244 ↗

(On Diff #132320)

Hm. From self review, I think this should be:

IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None ? IG_Rejected : IG_Inited;

736 ↗

(On Diff #132320)

technically I could drop the braces opened on this line. Would you like me to do that?

[mzeren-vmw]

• Add comments to IncludeGuardState.
• Fix re-initialization of IncludeGuard in UnwrappedLineParser::reset.
• Remove unnecessary block after if.
• Rebase

[krasimir]

Looks good! Thank you!

lib/Format/UnwrappedLineParser.cpp

244 ↗

(On Diff #132320)

Makes sense.

736 ↗

(On Diff #132320)

I like the braces.

cfe/trunk/lib/Format/UnwrappedLineParser.h

private:
   SmallVector<int, 8> PPLevelBranchCount;
 
   // Contains the number of branches per nesting level we are currently
   // in while parsing a preprocessor branch sequence.
   // This is used to update PPLevelBranchCount at the end of a branch
   // sequence.
   std::stack<int> PPChainBranchIndex;
 
-  // Contains the #ifndef condition for a potential include guard.
-  FormatToken *IfNdefCondition;
-  bool FoundIncludeGuardStart;
-  bool IncludeGuardRejected;
+  // Include guard search state. Used to fixup preprocessor indent levels
+  // so that include guards do not participate in indentation.
+  enum IncludeGuardState {
+    IG_Inited,
+    IG_IfNdefed,
+    IG_Defined,
+    IG_Found,
+    IG_Rejected,
+  };
+
+  // Current state of include guard search.
+  IncludeGuardState IncludeGuard;
+
+  // Points to the #ifndef condition for a potential include guard. Null unless
+  // IncludeGuardState == IG_IfNdefed.
+  FormatToken *IncludeGuardToken;
+
   // Contains the first start column where the source begins. This is zero for
   // normal source code and may be nonzero when formatting a code fragment that
   // does not start at the beginning of the file.
   unsigned FirstStartColumn;
 
   friend class ScopedLineState;
   friend class CompoundStatementIndenter;
 };

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

UnwrappedLineParser::UnwrappedLineParser(const FormatStyle &Style,
                                          const AdditionalKeywords &Keywords,
                                          unsigned FirstStartColumn,
                                          ArrayRef<FormatToken *> Tokens,
                                          UnwrappedLineConsumer &Callback)
     : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
       CurrentLines(&Lines), Style(Style), Keywords(Keywords),
       CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
       Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
-      IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
-      IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+      IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+                       ? IG_Rejected
+                       : IG_Inited),
+      IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = IG_Inited;
+  IncludeGuardToken = nullptr;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
   MustBreakBeforeNextToken = false;
   PreprocessorDirectives.clear();
   CurrentLines = &Lines;
   DeclarationScopeStack.clear();
   PPStack.clear();
   Line->FirstStartColumn = FirstStartColumn;
 }
 
 void UnwrappedLineParser::parse() {
   IndexedTokenSource TokenSource(AllTokens);
   Line->FirstStartColumn = FirstStartColumn;
   do {
     DEBUG(llvm::dbgs() << "----\n");
     reset();
     Tokens = &TokenSource;
     TokenSource.reset();
 
     readToken();
     parseFile();
+
+    // If we found an include guard then all preprocessor directives (other than
+    // the guard) are over-indented by one.
+    if (IncludeGuard == IG_Found)
+      for (auto &Line : Lines)
+        if (Line.InPPDirective && Line.Level > 0)
+          --Line.Level;
+
     // Create line with eof token.
     pushToken(FormatTok);
     addUnwrappedLine();
 
     for (SmallVectorImpl<UnwrappedLine>::iterator I = Lines.begin(),
                                                   E = Lines.end();
          I != E; ++I) {
       Callback.consumeUnwrappedLine(*I);
if (!IfDef && (FormatTok->is(tok::kw_false) || FormatTok->TokenText == "0"))
     Unreachable = true;
   if (IfDef && !IfNDef && FormatTok->TokenText == "SWIG")
     Unreachable = true;
   conditionalCompilationStart(Unreachable);
   FormatToken *IfCondition = FormatTok;
   // If there's a #ifndef on the first line, and the only lines before it are
   // comments, it could be an include guard.
   bool MaybeIncludeGuard = IfNDef;
-  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
     for (auto &Line : Lines) {
       if (!Line.Tokens.front().Tok->is(tok::comment)) {
         MaybeIncludeGuard = false;
-        IncludeGuardRejected = true;
+        IncludeGuard = IG_Rejected;
         break;
       }
     }
   }
   --PPBranchLevel;
   parsePPUnknown();
   ++PPBranchLevel;
-  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
-    IfNdefCondition = IfCondition;
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
+    IncludeGuard = IG_IfNdefed;
+    IncludeGuardToken = IfCondition;
+  }
 }
 
 void UnwrappedLineParser::parsePPElse() {
   // If a potential include guard has an #else, it's not an include guard.
-  if (FoundIncludeGuardStart && PPBranchLevel == 0)
-    FoundIncludeGuardStart = false;
+  if (IncludeGuard == IG_Defined && PPBranchLevel == 0)
+    IncludeGuard = IG_Rejected;
   conditionalCompilationAlternative();
   if (PPBranchLevel > -1)
     --PPBranchLevel;
   parsePPUnknown();
   ++PPBranchLevel;
 }
 
 void UnwrappedLineParser::parsePPElIf() { parsePPElse(); }
 
 void UnwrappedLineParser::parsePPEndIf() {
   conditionalCompilationEnd();
   parsePPUnknown();
   // If the #endif of a potential include guard is the last thing in the file,
-  // then we count it as a real include guard and subtract one from every
-  // preprocessor indent.
+  // then we found an include guard.
   unsigned TokenPosition = Tokens->getPosition();
   FormatToken *PeekNext = AllTokens[TokenPosition];
-  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof) &&
+  if (IncludeGuard == IG_Defined && PPBranchLevel == -1 &&
+      PeekNext->is(tok::eof) &&
       Style.IndentPPDirectives != FormatStyle::PPDIS_None)
-    for (auto &Line : Lines)
-      if (Line.InPPDirective && Line.Level > 0)
-        --Line.Level;
+    IncludeGuard = IG_Found;
 }
 
 void UnwrappedLineParser::parsePPDefine() {
   nextToken();
 
   if (FormatTok->Tok.getKind() != tok::identifier) {
+    IncludeGuard = IG_Rejected;
+    IncludeGuardToken = nullptr;
     parsePPUnknown();
     return;
   }
-  if (IfNdefCondition && IfNdefCondition->TokenText == FormatTok->TokenText) {
-    FoundIncludeGuardStart = true;
+
+  if (IncludeGuard == IG_IfNdefed &&
+      IncludeGuardToken->TokenText == FormatTok->TokenText) {
+    IncludeGuard = IG_Defined;
+    IncludeGuardToken = nullptr;
     for (auto &Line : Lines) {
       if (!Line.Tokens.front().Tok->isOneOf(tok::comment, tok::hash)) {
-        FoundIncludeGuardStart = false;
+        IncludeGuard = IG_Rejected;
         break;
       }
     }
   }
-  IfNdefCondition = nullptr;
+
   nextToken();
   if (FormatTok->Tok.getKind() == tok::l_paren &&
       FormatTok->WhitespaceRange.getBegin() ==
           FormatTok->WhitespaceRange.getEnd()) {
     parseParens();
   }
   if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
     Line->Level += PPBranchLevel + 1;

 
 void UnwrappedLineParser::parsePPUnknown() {
   do {
     nextToken();
   } while (!eof());
   if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
-  IfNdefCondition = nullptr;
 }
 
 // Here we blacklist certain tokens that are not usually the first token in an
 // unwrapped line. This is used in attempt to distinguish macro calls without
 // trailing semicolons from other constructs split to several lines.
 static bool tokenCanStartNewLine(const clang::Token &Tok) {
   // Semicolon can be a null-statement, l_square can be a start of a macro or
   // a C++11 attribute, but this doesn't seem to be common.

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("#ifndef NOT_GUARD\n"
                "#endif",
                Style);
   verifyFormat("#ifndef NOT_GUARD\n"
                "#  define NOT_GUARD\n"
                "code();\n"
                "#elif FOO\n"
                "#endif",
                Style);
+  // Non-identifier #define after potential include guard.
+  verifyFormat("#ifndef FOO\n"
+               "#  define 1\n"
+               "#endif\n",
+               Style);
+  // #if closes past last non-preprocessor line.
+  verifyFormat("#ifndef FOO\n"
+               "#define FOO\n"
+               "#if 1\n"
+               "int i;\n"
+               "#  define A 0\n"
+               "#endif\n"
+               "#endif\n",
+               Style);
   // FIXME: This doesn't handle the case where there's code between the
   // #ifndef and #define but all other conditions hold. This is because when
   // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
   // previous code line yet, so we can't detect it.
   EXPECT_EQ("#ifndef NOT_GUARD\n"
             "code();\n"
             "#define NOT_GUARD\n"
             "code();\n"