clang-format: fix block OpeningLineIndex around preprocessor

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.

Reviewers: krasimir, djasper

Reviewed By: krasimir

Subscribers: klimek, cfe-commits

Differential Revision: https://reviews.llvm.org/D35483

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@309369 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Format/UnwrappedLineParser.cpp b/lib/Format/UnwrappedLineParser.cpp
index 9bb9ed19183ccbaaa0b9add510ab278671ab2228..bbed83263ce0de7aea4bb1b3cc47833763320ea0 100644 (file)
--- a/lib/Format/UnwrappedLineParser.cpp
+++ b/lib/Format/UnwrappedLineParser.cpp
@@ -452,6 +452,21 @@ void UnwrappedLineParser::calculateBraceTypes(bool ExpectClassBody) {
   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) &&
@@ -459,16 +474,21 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   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()
+          ? (UnwrappedLine::kInvalidIndex)
+          : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
@@ -486,6 +506,8 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
     return;
   }
 
+  size_t PPEndHash = computePPHash();
+
   // Munch the closing brace.
   nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
 
@@ -495,11 +517,14 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->Level = InitialLevel;
-  Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
-  if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
-    // Update the opening line to add the forward reference as well
-    (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
-            CurrentLines->size() - 1;
+
+  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;
+    }
   }
 }
 
@@ -607,10 +632,15 @@ void UnwrappedLineParser::parsePPDirective() {
 }
 
 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) {
@@ -2400,7 +2430,7 @@ void UnwrappedLineParser::readToken(int LevelDifference) {
       FormatTok->MustBreakBefore = true;
     }
 
-    if (!PPStack.empty() && (PPStack.back() == PP_Unreachable) &&
+    if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
         !Line->InPPDirective) {
       continue;
     }

diff --git a/lib/Format/UnwrappedLineParser.h b/lib/Format/UnwrappedLineParser.h
index 5e1eef766196bce94916b707dedf449627b72428..47e41b3bb29dd3caa2dc3b53aa6d920568e473c3 100644 (file)
--- a/lib/Format/UnwrappedLineParser.h
+++ b/lib/Format/UnwrappedLineParser.h
@@ -160,6 +160,11 @@ private:
 
   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.
@@ -178,7 +183,7 @@ private:
 
   // 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.
@@ -211,8 +216,14 @@ private:
     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.

diff --git a/unittests/Format/NamespaceEndCommentsFixerTest.cpp b/unittests/Format/NamespaceEndCommentsFixerTest.cpp
index 92f34216293877f71547520698d19ab569d81721..fda8b4d69fe099745c25b4a213a08c68e0be3396 100644 (file)
--- a/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ b/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -509,6 +509,134 @@ TEST_F(NamespaceEndCommentsFixerTest,
                                     "}\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"