clang-format: fix block OpeningLineIndex around preprocessor

[Typz]

Summary

The current code would return an incorrect value when a preprocessor
directive is present immediately after the opening brace: this causes
the nanespace end comment fixer to break in some places, for exemple it
would not add the comment in this case:

namespace a {
#define FOO
}

Fixing the computation is simple enough, but it was breaking a feature,
as it would cause comments to be added also when the namespace
declaration was dependant on conditional compilation.

To fix this, a hash of the current preprocessor stack/branches is
computed at the beginning of parseBlock(), so that we explicitely do not
store the OpeningLineIndex when the beginning and end of the block are
not in the same preprocessor conditions.

Tthe hash is computed based on the line, but this could propbably be
improved by using the actual condition, so that clang-format would be
able to match multiple identical #ifdef blocks.

[krasimir]

Nice!

lib/Format/UnwrappedLineParser.cpp

461 ↗

(On Diff #106864)

@djasper: do you aware of some baked-in hash-combine functionality in llvm which this can use directly? If there is no, I'm happy with this.

unittests/Format/NamespaceEndCommentsFixerTest.cpp

556 ↗

(On Diff #106864)

Maybe also add some negative tests?

[Typz]

Add more tests

[krasimir]

lib/Format/UnwrappedLineParser.cpp

464 ↗

(On Diff #107030)

When I patch this, I get an UnwrappedLineParser.cpp:457:16 error: implicit instantiation of undefined template 'std::hash<clang::format::UnwrappedLineParser::PPBranchKind>'.

lib/Format/UnwrappedLineParser.h

158 ↗

(On Diff #107030)

Please add a short comment of why is the preprocessor hash needed.

213 ↗

(On Diff #107030)

This operator== is confusing. Please remove it.

[Typz]

Address review commentsx

[Typz]

lib/Format/UnwrappedLineParser.cpp

464 ↗

(On Diff #107030)

Seems to work fine with latest Ubuntu, but not on macos.

[krasimir]

Looks good! Thanks!
Please rebase with master before committing since we've been touching the same lines in UnwrappedLineParser.cpp.

cfe/trunk/lib/Format/UnwrappedLineParser.h

private:
   // not be taken in this round.
   void conditionalCompilationCondition(bool Unreachable);
   void conditionalCompilationStart(bool Unreachable);
   void conditionalCompilationAlternative();
   void conditionalCompilationEnd();
 
   bool isOnNewLine(const FormatToken &FormatTok);
 
+  // Compute hash of the current preprocessor branch.
+  // This is used to identify the different branches, and thus track if block
+  // open and close in the same branch.
+  size_t computePPHash() const;
+
   // FIXME: We are constantly running into bugs where Line.Level is incorrectly
   // subtracted from beyond 0. Introduce a method to subtract from Line.Level
   // and use that everywhere in the Parser.
   std::unique_ptr<UnwrappedLine> Line;
 
   // Comments are sorted into unwrapped lines by whether they are in the same
   // line as the previous token, or not. If not, they belong to the next token.
   // Since the next token might already be in a new unwrapped line, we need to
   // store the comments belonging to that token.
   SmallVector<FormatToken *, 1> CommentsBeforeNextToken;
   FormatToken *FormatTok;
   bool MustBreakBeforeNextToken;
 
   // The parsed lines. Only added to through \c CurrentLines.
   SmallVector<UnwrappedLine, 8> Lines;
 
   // Preprocessor directives are parsed out-of-order from other unwrapped lines.
   // Thus, we need to keep a list of preprocessor directives to be reported
-  // after an unwarpped line that has been started was finished.
+  // after an unwrapped line that has been started was finished.
   SmallVector<UnwrappedLine, 4> PreprocessorDirectives;
 
   // New unwrapped lines are added via CurrentLines.
   // Usually points to \c &Lines. While parsing a preprocessor directive when
   // there is an unfinished previous unwrapped line, will point to
   // \c &PreprocessorDirectives.
   SmallVectorImpl<UnwrappedLine> *CurrentLines;
 
private:
 
   // Represents preprocessor branch type, so we can find matching
   // #if/#else/#endif directives.
   enum PPBranchKind {
     PP_Conditional, // Any #if, #ifdef, #ifndef, #elif, block outside #if 0
     PP_Unreachable  // #if 0 or a conditional preprocessor block inside #if 0
   };
 
+  struct PPBranch {
+    PPBranch(PPBranchKind Kind, size_t Line) : Kind(Kind), Line(Line) {}
+    PPBranchKind Kind;
+    size_t Line;
+  };
+
   // Keeps a stack of currently active preprocessor branching directives.
-  SmallVector<PPBranchKind, 16> PPStack;
+  SmallVector<PPBranch, 16> PPStack;
 
   // The \c UnwrappedLineParser re-parses the code for each combination
   // of preprocessor branches that can be taken.
   // To that end, we take the same branch (#if, #else, or one of the #elif
   // branches) for each nesting level of preprocessor branches.
   // \c PPBranchLevel stores the current nesting level of preprocessor
   // branches during one pass over the code.
   int PPBranchLevel;

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   for (unsigned i = 0, e = LBraceStack.size(); i != e; ++i) {
     if (LBraceStack[i]->BlockKind == BK_Unknown)
       LBraceStack[i]->BlockKind = BK_Block;
   }
 
   FormatTok = Tokens->setPosition(StoredPosition);
 }
 
+template <class T>
+static inline void hash_combine(std::size_t &seed, const T &v) {
+  std::hash<T> hasher;
+  seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+}
+
+size_t UnwrappedLineParser::computePPHash() const {
+  size_t h = 0;
+  for (const auto &i : PPStack) {
+    hash_combine(h, size_t(i.Kind));
+    hash_combine(h, i.Line);
+  }
+  return h;
+}
+
 void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
                                      bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->BlockKind = BK_Block;
 
+  size_t PPStartHash = computePPHash();
+
   unsigned InitialLevel = Line->Level;
   nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
+  size_t NbPreprocessorDirectives =
+      CurrentLines == &Lines ? PreprocessorDirectives.size() : 0;
   addUnwrappedLine();
-  size_t OpeningLineIndex = CurrentLines->empty()
-                                ? (UnwrappedLine::kInvalidIndex)
-                                : (CurrentLines->size() - 1);
+  size_t OpeningLineIndex =
+      CurrentLines->empty()
+          : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
   if (AddLevel)
     ++Line->Level;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
     return;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->BlockKind = BK_Block;
     return;
   }
 
+  size_t PPEndHash = computePPHash();
+
   // Munch the closing brace.
   nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->Level = InitialLevel;
+
+  if (PPStartHash == PPEndHash) {
     Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
     if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
       // Update the opening line to add the forward reference as well
       (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
-            CurrentLines->size() - 1;
     }
   }
+}
 
 static bool isGoogScope(const UnwrappedLine &Line) {
   // FIXME: Closure-library specific stuff should not be hard-coded but be
   // configurable.
   if (Line.Tokens.size() < 4)
     return false;
   auto I = Line.Tokens.begin();
   if (I->Tok->TokenText != "goog")
case tok::pp_endif:
     break;
   default:
     parsePPUnknown();
     break;
   }
 }
 
 void UnwrappedLineParser::conditionalCompilationCondition(bool Unreachable) {
-  if (Unreachable || (!PPStack.empty() && PPStack.back() == PP_Unreachable))
-    PPStack.push_back(PP_Unreachable);
+  size_t Line = CurrentLines->size();
+  if (CurrentLines == &PreprocessorDirectives)
+    Line += Lines.size();
+
+  if (Unreachable ||
+      (!PPStack.empty() && PPStack.back().Kind == PP_Unreachable))
+    PPStack.push_back({PP_Unreachable, Line});
   else
-    PPStack.push_back(PP_Conditional);
+    PPStack.push_back({PP_Conditional, Line});
 }
 
 void UnwrappedLineParser::conditionalCompilationStart(bool Unreachable) {
   ++PPBranchLevel;
   assert(PPBranchLevel >= 0 && PPBranchLevel <= (int)PPLevelBranchIndex.size());
   if (PPBranchLevel == (int)PPLevelBranchIndex.size()) {
     PPLevelBranchIndex.push_back(0);
     PPLevelBranchCount.push_back(0);
while (FormatTok->Type == TT_ConflictStart ||
         conditionalCompilationAlternative();
       } else if (FormatTok->Type == TT_ConflictEnd) {
         conditionalCompilationEnd();
       }
       FormatTok = Tokens->getNextToken();
       FormatTok->MustBreakBefore = true;
     }
 
-    if (!PPStack.empty() && (PPStack.back() == PP_Unreachable) &&
+    if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
         !Line->InPPDirective) {
       continue;
     }
 
     if (!FormatTok->Tok.is(tok::comment)) {
       distributeComments(Comments, FormatTok);
       Comments.clear();
       return;

cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp

EXPECT_EQ("#ifdef 1\n"
                                     "#elseif\n"
                                     "namespace B {\n"
                                     "#endif\n"
                                     "  int i;\n"
                                     "}\n"
                                     "}\n"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddEndCommentForNamespacesAroundMacros) {
+  // Conditional blocks around are fine
+  EXPECT_EQ("namespace A {\n"
+            "#if 1\n"
+            "int i;\n"
+            "#endif\n"
+            "}// namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "#if 1\n"
+                                    "int i;\n"
+                                    "#endif\n"
+                                    "}"));
+  EXPECT_EQ("#if 1\n"
+            "#endif\n"
+            "namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A",
+            fixNamespaceEndComments("#if 1\n"
+                                    "#endif\n"
+                                    "namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A\n"
+            "#if 1\n"
+            "#endif",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}\n"
+                                    "#if 1\n"
+                                    "#endif"));
+  EXPECT_EQ("#if 1\n"
+            "namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A\n"
+            "#endif",
+            fixNamespaceEndComments("#if 1\n"
+                                    "namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}\n"
+                                    "#endif"));
+
+  // Macro definition has no impact
+  EXPECT_EQ("namespace A {\n"
+            "#define FOO\n"
+            "int i;\n"
+            "}// namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "#define FOO\n"
+                                    "int i;\n"
+                                    "}"));
+  EXPECT_EQ("#define FOO\n"
+            "namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A",
+            fixNamespaceEndComments("#define FOO\n"
+                                    "namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A\n"
+            "#define FOO\n",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}\n"
+                                    "#define FOO\n"));
+
+  // No replacement if open & close in different conditional blocks
+  EXPECT_EQ("#if 1\n"
+            "namespace A {\n"
+            "#endif\n"
+            "int i;\n"
+            "int j;\n"
+            "#if 1\n"
+            "}\n"
+            "#endif",
+            fixNamespaceEndComments("#if 1\n"
+                                    "namespace A {\n"
+                                    "#endif\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "#if 1\n"
+                                    "}\n"
+                                    "#endif"));
+  EXPECT_EQ("#ifdef A\n"
+            "namespace A {\n"
+            "#endif\n"
+            "int i;\n"
+            "int j;\n"
+            "#ifdef B\n"
+            "}\n"
+            "#endif",
+            fixNamespaceEndComments("#ifdef A\n"
+                                    "namespace A {\n"
+                                    "#endif\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "#ifdef B\n"
+                                    "}\n"
+                                    "#endif"));
+
+  // No replacement inside unreachable conditional block
+  EXPECT_EQ("#if 0\n"
+            "namespace A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}\n"
+            "#endif",
+            fixNamespaceEndComments("#if 0\n"
+                                    "namespace A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}\n"
+                                    "#endif"));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest,
        DoesNotAddEndCommentForNamespacesInMacroDeclarations) {
   EXPECT_EQ("#ifdef 1\n"
             "namespace A {\n"
             "#elseif\n"
             "namespace B {\n"
             "#endif\n"
             "  int i;\n"