[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6482383e50a4: [clang-format] FixNamespaceComments does not 
understand namespace aliases (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

Files:
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -1185,6 +1185,82 @@
 "}\n",
 Style));
 }
+
+TEST_F(ShortNamespaceLinesTest, NamespaceAlias) {
+  auto Style = getLLVMStyle();
+
+  EXPECT_EQ("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ(
+  "namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  fixNamespaceEndComments("namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  Style));
+
+  EXPECT_EQ("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+  EXPECT_EQ("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}// namespace n\n",
+fixNamespaceEndComments("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/NamespaceEndCommentsFixer.cpp
===
--- clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -180,9 +180,13 @@
   if (NamespaceTok->is(tok::l_brace)) {
 // "namespace" keyword can be on the line preceding '{', e.g. in styles
 // where BraceWrapping.AfterNamespace is true.
-if (StartLineIndex > 0)
+if (StartLineIndex > 0) {
   NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+  if (AnnotatedLines[StartLineIndex - 1]->endsWith(tok::semi))
+return nullptr;
+}
   }
+
   return NamespaceTok->getNamespaceToken();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 394163.
MyDeveloperDay added a comment.

use endsWith()


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

Files:
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -1185,6 +1185,82 @@
 "}\n",
 Style));
 }
+
+TEST_F(ShortNamespaceLinesTest, NamespaceAlias) {
+  auto Style = getLLVMStyle();
+
+  EXPECT_EQ("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ(
+  "namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  fixNamespaceEndComments("namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  Style));
+
+  EXPECT_EQ("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+  EXPECT_EQ("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}// namespace n\n",
+fixNamespaceEndComments("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/NamespaceEndCommentsFixer.cpp
===
--- clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -180,9 +180,13 @@
   if (NamespaceTok->is(tok::l_brace)) {
 // "namespace" keyword can be on the line preceding '{', e.g. in styles
 // where BraceWrapping.AfterNamespace is true.
-if (StartLineIndex > 0)
+if (StartLineIndex > 0) {
   NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+  if (AnnotatedLines[StartLineIndex - 1]->endsWith(tok::semi))
+return nullptr;
+}
   }
+
   return NamespaceTok->getNamespaceToken();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:185-189
+  const FormatToken *Previous = AnnotatedLines[StartLineIndex - 1]->Last;
+  while (Previous && Previous->is(tok::comment)) {
+Previous = Previous->Previous;
+  }
+  if (Previous && Previous->is(tok::semi))

Something like below will do:
`  if (AnnotatedLines[StartLineIndex - 1]->endsWith(tok::semi))`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

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

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

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

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 394010.
MyDeveloperDay added a comment.

Look backwards from the { rather than scanning the namespace


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

Files:
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -1185,6 +1185,82 @@
 "}\n",
 Style));
 }
+
+TEST_F(ShortNamespaceLinesTest, NamespaceAlias) {
+  auto Style = getLLVMStyle();
+
+  EXPECT_EQ("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; // comment\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; /* comment */\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+
+  EXPECT_EQ(
+  "namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  fixNamespaceEndComments("namespace n = nn; /* comment */ /* comment2 */\n"
+  "{\n"
+  "  int i;\n"
+  "  int j;\n"
+  "}\n",
+  Style));
+
+  EXPECT_EQ("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn; {\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+  EXPECT_EQ("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}// namespace n\n",
+fixNamespaceEndComments("int foo;\n"
+"namespace n\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/NamespaceEndCommentsFixer.cpp
===
--- clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -180,9 +180,17 @@
   if (NamespaceTok->is(tok::l_brace)) {
 // "namespace" keyword can be on the line preceding '{', e.g. in styles
 // where BraceWrapping.AfterNamespace is true.
-if (StartLineIndex > 0)
+if (StartLineIndex > 0) {
   NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+  const FormatToken *Previous = AnnotatedLines[StartLineIndex - 1]->Last;
+  while (Previous && Previous->is(tok::comment)) {
+Previous = Previous->Previous;
+  }
+  if (Previous && Previous->is(tok::semi))
+return nullptr;
+}
   }
+
   return NamespaceTok->getNamespaceToken();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:59
+  // Namespace alias
+  if (Tok->isOneOf(tok::equal, tok::semi))
+return false;

Should only be equal, right?
Otherwise please add a test for semi.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

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

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:27
 // Returns "" for anonymous namespace.
-std::string computeName(const FormatToken *NamespaceTok) {
+bool computeName(const FormatToken *NamespaceTok, std::string ) {
   assert(NamespaceTok &&

It seems to be a good place for optional, nope?



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:183
   const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
   if (NamespaceTok->is(tok::l_brace)) {
 // "namespace" keyword can be on the line preceding '{', e.g. in styles

Maybe just checking previous (non-comment) token to be not a semicolon would be 
enough?



Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:250
+std::string NamespaceName;
+if (!computeName(NamespaceTok, NamespaceName)) {
+  // Its likely a namespace alias.

As an alternative, couldn't we avoid computing the name for namespace aliases 
by searching the next semicolon (don't compute) or l_brace (compute)?



Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:1189
+
+TEST_F(ShortNamespaceLinesTest, NameSpaceAlias) {
+  auto Style = getLLVMStyle();

Nit: `NamespaceAlias` with small 's'.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115647/new/

https://reviews.llvm.org/D115647

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

[PATCH] D115647: [clang-format] FixNamespaceComments does not understand namespace aliases

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: HazardyKnusperkeks, curdeius, owenpan.
MyDeveloperDay added projects: clang, clang-format.
Herald added a subscriber: jeroen.dobbelaere.
MyDeveloperDay requested review of this revision.

https://github.com/llvm/llvm-project/issues/35876

Ensure a namespace alias doesn't get incorrectly identifier as a namespace

  namespace nn {}
  void f() {
namespace n = nn;
{
  int i;
  int j;
} // namespace n=nn;
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115647

Files:
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp


Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -1185,6 +1185,22 @@
 "}\n",
 Style));
 }
+
+TEST_F(ShortNamespaceLinesTest, NameSpaceAlias) {
+  auto Style = getLLVMStyle();
+
+  EXPECT_EQ("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/NamespaceEndCommentsFixer.cpp
===
--- clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -24,11 +24,11 @@
 namespace {
 // Computes the name of a namespace given the namespace token.
 // Returns "" for anonymous namespace.
-std::string computeName(const FormatToken *NamespaceTok) {
+bool computeName(const FormatToken *NamespaceTok, std::string ) {
   assert(NamespaceTok &&
  NamespaceTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
  "expecting a namespace token");
-  std::string name = "";
+  name = "";
   const FormatToken *Tok = NamespaceTok->getNextNonComment();
   if (NamespaceTok->is(TT_NamespaceMacro)) {
 // Collects all the non-comment tokens between opening parenthesis
@@ -55,10 +55,13 @@
   name += Tok->TokenText;
   if (Tok->is(tok::kw_inline))
 name += " ";
+  // Namespace alias
+  if (Tok->isOneOf(tok::equal, tok::semi))
+return false;
   Tok = Tok->getNextNonComment();
 }
   }
-  return name;
+  return true;
 }
 
 std::string computeEndCommentText(StringRef NamespaceName, bool AddNewline,
@@ -242,7 +245,13 @@
 }
 if (StartLineIndex == SIZE_MAX)
   StartLineIndex = EndLine->MatchingOpeningBlockLineIndex;
-std::string NamespaceName = computeName(NamespaceTok);
+
+std::string NamespaceName;
+if (!computeName(NamespaceTok, NamespaceName)) {
+  // Its likely a namespace alias.
+  continue;
+}
+
 if (Style.CompactNamespaces) {
   if (CompactedNamespacesCount == 0)
 NamespaceTokenText = NamespaceTok->TokenText;


Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -1185,6 +1185,22 @@
 "}\n",
 Style));
 }
+
+TEST_F(ShortNamespaceLinesTest, NameSpaceAlias) {
+  auto Style = getLLVMStyle();
+
+  EXPECT_EQ("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+fixNamespaceEndComments("namespace n = nn;\n"
+"{\n"
+"  int i;\n"
+"  int j;\n"
+"}\n",
+Style));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/NamespaceEndCommentsFixer.cpp
===
--- clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -24,11 +24,11 @@
 namespace {
 // Computes the name of a namespace given the namespace token.
 // Returns "" for anonymous namespace.
-std::string computeName(const FormatToken *NamespaceTok) {
+bool computeName(const FormatToken *NamespaceTok, std::string ) {
   assert(NamespaceTok &&
  NamespaceTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
  "expecting a namespace token");
-  std::string name = "";
+  name = "";
   const FormatToken *Tok =