[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL309369: clang-format: fix block OpeningLineIndex around 
preprocessor (authored by Typz).

Changed prior to commit:
  https://reviews.llvm.org/D35483?vs=107843=108598#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35483

Files:
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.h
  cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: cfe/trunk/lib/Format/UnwrappedLineParser.h
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.h
+++ cfe/trunk/lib/Format/UnwrappedLineParser.h
@@ -160,6 +160,11 @@
 
   bool isOnNewLine(const FormatToken );
 
+  // 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 @@
 
   // 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 PreprocessorDirectives;
 
   // New unwrapped lines are added via CurrentLines.
@@ -211,8 +216,14 @@
 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 PPStack;
+  SmallVector PPStack;
 
   // The \c UnwrappedLineParser re-parses the code for each combination
   // of preprocessor branches that can be taken.
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -452,23 +452,43 @@
   FormatTok = Tokens->setPosition(StoredPosition);
 }
 
+template 
+static inline void hash_combine(std::size_t , const T ) {
+  std::hash hasher;
+  seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+}
+
+size_t UnwrappedLineParser::computePPHash() const {
+  size_t h = 0;
+  for (const auto  : 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 ==  ? 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 @@
 return;
   }
 
+  size_t PPEndHash = computePPHash();
+
   // Munch the closing brace.
   nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
 
@@ -495,11 +517,14 @@
   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::conditionalCompilationCondition(bool Unreachable) {

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

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


https://reviews.llvm.org/D35483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:464
+  for (const auto  : PPStack) {
+hash_combine(h, i.Kind);
+hash_combine(h, i.Line);

krasimir wrote:
> When I patch this, I get an `UnwrappedLineParser.cpp:457:16 error: implicit 
> instantiation of undefined template 
> 'std::hash'`. 
Seems to work fine with latest Ubuntu, but not on macos.


https://reviews.llvm.org/D35483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 107843.
Typz marked 3 inline comments as done.
Typz added a comment.

Address review commentsx


https://reviews.llvm.org/D35483

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -509,6 +509,134 @@
 "}\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"
+

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:464
+  for (const auto  : PPStack) {
+hash_combine(h, i.Kind);
+hash_combine(h, i.Line);

When I patch this, I get an `UnwrappedLineParser.cpp:457:16 error: implicit 
instantiation of undefined template 
'std::hash'`. 



Comment at: lib/Format/UnwrappedLineParser.h:158
   bool isOnNewLine(const FormatToken );
+  size_t computePPHash() const;
 

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



Comment at: lib/Format/UnwrappedLineParser.h:213
+PPBranch(PPBranchKind Kind, size_t Line) : Kind(Kind), Line(Line) {}
+bool operator==(PPBranchKind Kind) const { return Kind == this->Kind; }
+PPBranchKind Kind;

This `operator==` is confusing. Please remove it.


https://reviews.llvm.org/D35483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 107030.
Typz marked an inline comment as done.
Typz added a comment.

Add more tests


https://reviews.llvm.org/D35483

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -509,6 +509,134 @@
 "}\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"
+

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Nice!




Comment at: lib/Format/UnwrappedLineParser.cpp:461
+
+size_t UnwrappedLineParser::computePPHash() const {
+  size_t h = 0;

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



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:556
+}
+
 TEST_F(NamespaceEndCommentsFixerTest,

Maybe also add some negative tests?


https://reviews.llvm.org/D35483



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35483: clang-format: fix block OpeningLineIndex around preprocessor

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Herald added a subscriber: klimek.

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.


https://reviews.llvm.org/D35483

Files:
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -509,6 +509,51 @@
 "}\n"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddEndCommentForNamespacesAroundMacros) {
+  EXPECT_EQ("namespace A {\n"
+"#if 0\n"
+"int i;\n"
+"#endif\n"
+"}// namespace A",
+fixNamespaceEndComments("namespace A {\n"
+"#if 0\n"
+"int i;\n"
+"#endif\n"
+"}"));
+  EXPECT_EQ("#if 0\n"
+"#endif\n"
+"namespace A {\n"
+"int i;\n"
+"int j;\n"
+"}// namespace A",
+fixNamespaceEndComments("#if 0\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 0\n"
+"#endif",
+fixNamespaceEndComments("namespace A {\n"
+"int i;\n"
+"int j;\n"
+"}\n"
+"#if 0\n"
+"#endif"));
+  EXPECT_EQ("namespace A {\n"
+"#define FOO\n"
+"int i;\n"
+"}// namespace A",
+fixNamespaceEndComments("namespace A {\n"
+"#define FOO\n"
+"int i;\n"
+"}"));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest,
DoesNotAddEndCommentForNamespacesInMacroDeclarations) {
   EXPECT_EQ("#ifdef 1\n"
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -155,6 +155,7 @@
   void conditionalCompilationEnd();
 
   bool isOnNewLine(const FormatToken );
+  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
@@ -174,7 +175,7 @@
 
   // 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 PreprocessorDirectives;
 
   // New unwrapped lines are added via CurrentLines.
@@ -207,8 +208,15 @@
 PP_Unreachable  // #if 0 or a conditional preprocessor block inside #if 0
   };
 
+  struct PPBranch {
+PPBranch(PPBranchKind Kind, size_t Line) : Kind(Kind), Line(Line) {}
+bool operator==(PPBranchKind Kind) const { return Kind == this->Kind; }
+PPBranchKind Kind;
+size_t Line;
+  };
+
   // Keeps a stack of currently active preprocessor branching directives.
-  SmallVector PPStack;
+  SmallVector PPStack;
 
   // The \c UnwrappedLineParser re-parses the code for each combination
   // of preprocessor branches that can be taken.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -452,23 +452,43 @@
   FormatTok =