[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362740: clang-format: better handle namespace macros 
(authored by Typz, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D37813

Files:
  cfe/trunk/docs/ClangFormatStyleOptions.rst
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/FormatToken.h
  cfe/trunk/lib/Format/FormatTokenLexer.cpp
  cfe/trunk/lib/Format/NamespaceEndCommentsFixer.cpp
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/lib/Format/TokenAnnotator.h
  cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -1870,9 +1870,117 @@
Style));
 }
 
+TEST_F(FormatTest, NamespaceMacros) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  verifyFormat("TESTSUITE(A) {\n"
+   "int foo();\n"
+   "} // TESTSUITE(A)",
+   Style);
+
+  verifyFormat("TESTSUITE(A, B) {\n"
+   "int foo();\n"
+   "} // TESTSUITE(A)",
+   Style);
+
+  // Properly indent according to NamespaceIndentation style
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  verifyFormat("TESTSUITE(A) {\n"
+   "  int foo();\n"
+   "} // TESTSUITE(A)",
+   Style);
+  verifyFormat("TESTSUITE(A) {\n"
+   "  namespace B {\n"
+   "int foo();\n"
+   "  } // namespace B\n"
+   "} // TESTSUITE(A)",
+   Style);
+  verifyFormat("namespace A {\n"
+   "  TESTSUITE(B) {\n"
+   "int foo();\n"
+   "  } // TESTSUITE(B)\n"
+   "} // namespace A",
+   Style);
+
+  Style.NamespaceIndentation = FormatStyle::NI_Inner;
+  verifyFormat("TESTSUITE(A) {\n"
+   "TESTSUITE(B) {\n"
+   "  int foo();\n"
+   "} // TESTSUITE(B)\n"
+   "} // TESTSUITE(A)",
+   Style);
+  verifyFormat("TESTSUITE(A) {\n"
+   "namespace B {\n"
+   "  int foo();\n"
+   "} // namespace B\n"
+   "} // TESTSUITE(A)",
+   Style);
+  verifyFormat("namespace A {\n"
+   "TESTSUITE(B) {\n"
+   "  int foo();\n"
+   "} // TESTSUITE(B)\n"
+   "} // namespace A",
+   Style);
+
+  // Properly merge namespace-macros blocks in CompactNamespaces mode
+  Style.NamespaceIndentation = FormatStyle::NI_None;
+  Style.CompactNamespaces = true;
+  verifyFormat("TESTSUITE(A) { TESTSUITE(B) {\n"
+   "}} // TESTSUITE(A::B)",
+   Style);
+
+  EXPECT_EQ("TESTSUITE(out) { TESTSUITE(in) {\n"
+"}} // TESTSUITE(out::in)",
+format("TESTSUITE(out) {\n"
+   "TESTSUITE(in) {\n"
+   "} // TESTSUITE(in)\n"
+   "} // TESTSUITE(out)",
+   Style));
+
+  EXPECT_EQ("TESTSUITE(out) { TESTSUITE(in) {\n"
+"}} // TESTSUITE(out::in)",
+format("TESTSUITE(out) {\n"
+   "TESTSUITE(in) {\n"
+   "} // TESTSUITE(in)\n"
+   "} // TESTSUITE(out)",
+   Style));
+
+  // Do not merge different namespaces/macros
+  EXPECT_EQ("namespace out {\n"
+"TESTSUITE(in) {\n"
+"} // TESTSUITE(in)\n"
+"} // namespace out",
+format("namespace out {\n"
+   "TESTSUITE(in) {\n"
+   "} // TESTSUITE(in)\n"
+   "} // namespace out",
+   Style));
+  EXPECT_EQ("TESTSUITE(out) {\n"
+"namespace in {\n"
+"} // namespace in\n"
+"} // TESTSUITE(out)",
+format("TESTSUITE(out) {\n"
+   "namespace in {\n"
+   "} // namespace in\n"
+   "} // TESTSUITE(out)",
+   Style));
+  Style.NamespaceMacros.push_back("FOOBAR");
+  EXPECT_EQ("TESTSUITE(out) {\n"
+"FOOBAR(in) {\n"
+"} // FOOBAR(in)\n"
+"} // TESTSUITE(out)",
+format("TESTSUITE(out) {\n"
+   "FOOBAR(in) {\n"
+   "} // FOOBAR(in)\n"
+   "} // TESTSUITE(out)",
+   Style));
+}
+
 TEST_F(FormatTest, FormatsCompactNamespaces) {
   FormatStyle Style = getLLVMStyle();
   Style.CompactNamespaces = true;
+  

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 203425.
Typz added a comment.

Rebase


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -52,6 +52,7 @@
 "int i;\n"
 "int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "int j;\n"
@@ -248,6 +249,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "int i;\n"
@@ -380,6 +460,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

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

LG


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked 2 inline comments as done.
Typz added inline comments.



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:526
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
 "} // namespace A",

klimek wrote:
> Typz wrote:
> > Should I also fix these tests?
> > 
> > They already existed before this patch, but do not follow LLVM namespace 
> > indent style either.
> If you don't mind, that would be useful - doing it in a separate CL is fine, 
> so we keep this one focused (and no review needed).
Done


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

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

- Rebase
- Fix NamespaceEndCommentsFixerTest to match LLVM indent style


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/TokenAnnotator.h
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -52,6 +52,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -248,6 +249,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"int i;\n"
+"int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"int i;\n"
+"int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -380,6 +460,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"int i;\n"
+

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:526
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
 "} // namespace A",

Typz wrote:
> Should I also fix these tests?
> 
> They already existed before this patch, but do not follow LLVM namespace 
> indent style either.
If you don't mind, that would be useful - doing it in a separate CL is fine, so 
we keep this one focused (and no review needed).


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz marked 2 inline comments as done.
Typz added inline comments.



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:526
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
 "} // namespace A",

Should I also fix these tests?

They already existed before this patch, but do not follow LLVM namespace indent 
style either.



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:582
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // TESTSUITE()",

klimek wrote:
> All of the fixNamespaceEndComments tests are indented, but standard llvm 
> style doesn't indent in namespaces at all iiuc.
Ok, understood. I can fix that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:582
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // TESTSUITE()",

All of the fixNamespaceEndComments tests are indented, but standard llvm style 
doesn't indent in namespaces at all iiuc.


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Ah, wow, I see what confused me now:
>  In the fixNamespaceEndComment tests you use an indent that clang-format 
> would not produce. Can you please fix that, otherwise I find this super 
> confusing :)

Can you point a specific exemple (in code) ?

There are many tests related to fixNamespaceEndComment(), which look fine to me 
and which in many cases are copied from similarly indented test cases with 
standard namespace's...
(Or was it an earlier problem, and I should fix all these existing test cases 
as well?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D37813#1512317 , @Typz wrote:

> The patch goal is indeed to indent the content of namespace-macro blocks like 
> the content of any 'regular' namespace. So it should look like the content of 
> a namespace, possibly depending on the choose style options. To sumarize, 
> here is a detailed summary of the observable differences between macros vs 
> normal namespace.
>
> **Without this patch**, clang-format does not understand the macro call 
> actually defines a namespace, thus:
>
> - the content of the namespace-macro block is indented, even when 
> `Style.NamespaceIndentation = FormatStyle::NI_None` ``` lang=cpp TESTSUITE(A) 
> { int i;  // <--- should not be indented } namespace B { int 
> j; } ```
> - similarly for nested namespaces, when  `Style.NamespaceIndentation = 
> FormatStyle::NI_Inner` : ``` lang=cpp TESTSUITE(A) { TESTSUITE(B) { int i;
> // <--- should be indented 2-chars only } } namespace C { 
> namespace D { int j; } } ```
> - There is no automatic fix of namespace end comment when 
> `Style.FixNamespaceComments = true` ``` lang=cpp TESTSUITE(A) { int i; }  
>// <--- should have a namespace end comment namespace B { 
> int j; } // namespace B ```
> - Multiple nested namespaces are not compacted when `Style.CompactNamespaces 
> = true` ``` lang=cpp TESTSUITE(A) { TESTSUITE(B) { //<--- should 
> be merged with previous line int i; } } // <--- 
> should be merged with previous line (and have a "combined" namespace end 
> comment) namespace A { namespace B { int j; } // namespace A::B ```
>
>   ---
>
>   **This patch fixes all these points**, which hopefully leads to the exact 
> same formatting when using the namespace keyword or the namespace-macros: ``` 
> lang=cpp // Style.NamespaceIndentation = FormatStyle::NI_None TESTSUITE(A) { 
> int i; } namespace B { int j; }
>
>   // Style.NamespaceIndentation = FormatStyle::NI_Inner TESTSUITE(A) { 
> TESTSUITE(B) { int i; } } namespace C { namespace D { int j; } }
>
>   // Style.FixNamespaceComments = true TESTSUITE(A) { int i; } // 
> TESTSUITE(A) namespace B { int j; } // namespace B
>
>   // Style.CompactNamespaces = true TESTSUITE(A) { TESTSUITE(B) { int i; }} 
> // TESTSUITE(A::B) namespace A { namespace B { int j; }} // namespace A::B ```
>
>   I did not see any issue in my testing or reviewing the code, but if you see 
> a place in the tests where it does not match this, please point it out !


Ah, wow, I see what confused me now:
In the fixNamespaceEndComment tests you use an indent that clang-format would 
not produce. Can you please fix that, otherwise I find this super confusing :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

The patch goal is indeed to indent the content of namespace-macro blocks like 
the content of any 'regular' namespace. So it should look like the content of a 
namespace, possibly depending on the choose style options. To sumarize, here is 
a detailed summary of the observable differences between macros vs normal 
namespace.

**Without this patch**, clang-format does not understand the macro call 
actually defines a namespace, thus:

- the content of the namespace-macro block is indented, even when 
`Style.NamespaceIndentation = FormatStyle::NI_None`

  TESTSUITE(A) {
int i;  // <--- should not be indented
  }
  namespace B {
  int j;
  }



- similarly for nested namespaces, when  `Style.NamespaceIndentation = 
FormatStyle::NI_Inner` :

  TESTSUITE(A) {
  TESTSUITE(B) {
  int i;// <--- should be indented 2-chars only
  }
  }
  namespace C {
  namespace D {
int j;
  }
  }

- There is no automatic fix of namespace end comment when 
`Style.FixNamespaceComments = true`

  TESTSUITE(A) {
int i;
  } // <--- should have a namespace end comment
  namespace B {
int j;
  } // namespace B

- Multiple nested namespaces are not compacted when `Style.CompactNamespaces = 
true`

  TESTSUITE(A) {
  TESTSUITE(B) { //<--- should be merged with previous line
  int i;
  }
  } // <--- should be merged with previous line (and 
have a "combined" namespace end comment)
  namespace A { namespace B {
  int j;
  } // namespace A::B



---

**This patch fixes all these points**, which hopefully leads to the exact same 
formatting when using the namespace keyword or the namespace-macros:

  // Style.NamespaceIndentation = FormatStyle::NI_None
  TESTSUITE(A) {
  int i;
  }
  namespace B {
  int j;
  }
  
  // Style.NamespaceIndentation = FormatStyle::NI_Inner
  TESTSUITE(A) {
  TESTSUITE(B) {
int i;
  }
  }
  namespace C {
  namespace D {
int j;
  }
  }
  
  // Style.FixNamespaceComments = true
  TESTSUITE(A) {
int i;
  } // TESTSUITE(A)
  namespace B {
int j;
  } // namespace B
  
  // Style.CompactNamespaces = true
  TESTSUITE(A) { TESTSUITE(B) {
int i;
  }} // TESTSUITE(A::B)
  namespace A { namespace B {
int j;
  }} // namespace A::B

I did not see any issue in my testing or reviewing the code, but if you see a 
place in the tests where it does not match this, please point it out !


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D37813#1254891 , @Typz wrote:

> In D37813#1254865 , @klimek wrote:
>
> > In D37813#1253813 , @Typz wrote:
> >
> > > In D37813#1184051 , @klimek 
> > > wrote:
> > >
> > > > The problem here is that we have different opinions on how the 
> > > > formatting on namespace macros should behave in the first place- I 
> > > > think they should behave like namespaces, you want them to be formatted 
> > > > differently.
> > > >  Given that you want full control over the formatting of those macros, 
> > > > and them not actually be formatted exactly like namespaces or classes, 
> > > > I think we need a more generic mechanism for you to express that.
> > >
> > >
> > > Not sure what you mean here. I want them to behave like namespaces as 
> > > well, this is actually the use case I have... As implemented, they indeed 
> > > behave exactly like namespaces :
> > >
> > >   TESTSUITE(a) {   namespace a {
> > >   } // TESTSUITE(a)} // namespace a
> > >   VS
> > >   TESTSUITE(a) { TESTSUITE(b) {namespace a { namespace b {
> > >   } // TESTSUITE(a::b) }} // namespace a::b
> >
> >
> > I thought we had the discussion upthread that they indent differently from 
> > namespaces. I'm working on a patch that allows you to configure macros.
>
>
> The current behavior is that they indent differently from namespace, because 
> clang does not know these macros are conceptually equivalent to namespaces. 
> And this patch actually fixes that, letting clang know they are actually 
> macros.


You say "and this patch fixes that", but in your test cases it looks like the 
indent of things within the namespace is not like the indent for namespaces. 
I'm thoroughly confused.


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.
Herald added a project: clang.

ping ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D37813#1254865, @klimek wrote:

> In https://reviews.llvm.org/D37813#1253813, @Typz wrote:
>
> > In https://reviews.llvm.org/D37813#1184051, @klimek wrote:
> >
> > > The problem here is that we have different opinions on how the formatting 
> > > on namespace macros should behave in the first place- I think they should 
> > > behave like namespaces, you want them to be formatted differently.
> > >  Given that you want full control over the formatting of those macros, 
> > > and them not actually be formatted exactly like namespaces or classes, I 
> > > think we need a more generic mechanism for you to express that.
> >
> >
> > Not sure what you mean here. I want them to behave like namespaces as well, 
> > this is actually the use case I have... As implemented, they indeed behave 
> > exactly like namespaces :
> >
> >   TESTSUITE(a) {   namespace a {
> >   } // TESTSUITE(a)} // namespace a
> >   VS
> >   TESTSUITE(a) { TESTSUITE(b) {namespace a { namespace b {
> >   } // TESTSUITE(a::b) }} // namespace a::b
>
>
> I thought we had the discussion upthread that they indent differently from 
> namespaces. I'm working on a patch that allows you to configure macros.


The current behavior is that they indent differently from namespace, because 
clang does not know these macros are conceptually equivalent to namespaces. And 
this patch actually fixes that, letting clang know they are actually macros.


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#1253813, @Typz wrote:

> In https://reviews.llvm.org/D37813#1184051, @klimek wrote:
>
> > The problem here is that we have different opinions on how the formatting 
> > on namespace macros should behave in the first place- I think they should 
> > behave like namespaces, you want them to be formatted differently.
> >  Given that you want full control over the formatting of those macros, and 
> > them not actually be formatted exactly like namespaces or classes, I think 
> > we need a more generic mechanism for you to express that.
>
>
> Not sure what you mean here. I want them to behave like namespaces as well, 
> this is actually the use case I have... As implemented, they indeed behave 
> exactly like namespaces :
>
>   TESTSUITE(a) {   namespace a {
>   } // TESTSUITE(a)} // namespace a
>   VS
>   TESTSUITE(a) { TESTSUITE(b) {namespace a { namespace b {
>   } // TESTSUITE(a::b) }} // namespace a::b


I thought we had the discussion upthread that they indent differently from 
namespaces. I'm working on a patch that allows you to configure macros.


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D37813#1184051, @klimek wrote:

> The problem here is that we have different opinions on how the formatting on 
> namespace macros should behave in the first place- I think they should behave 
> like namespaces, you want them to be formatted differently.
>  Given that you want full control over the formatting of those macros, and 
> them not actually be formatted exactly like namespaces or classes, I think we 
> need a more generic mechanism for you to express that.


Not sure what you mean here. I want them to behave like namespaces as well, 
this is actually the use case I have... As implemented, they indeed behave 
exactly like namespaces :

  TESTSUITE(a) {   namespace a {
  } // TESTSUITE(a)} // namespace a
  VS
  TESTSUITE(a) { TESTSUITE(b) {namespace a { namespace b {
  } // TESTSUITE(a::b) }} // namespace a::b

(as long as there is a single argument. When multiple arguments are used, I add 
to choose a heuristic...)

As far as I understand, the divergence is that you would want something to 
"match" the implementation of the macro, while I propose a simpler heuristic, 
which should work fine for namespaces...


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

The problem here is that we have different opinions on how the formatting on 
namespace macros should behave in the first place- I think they should behave 
like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them 
not actually be formatted exactly like namespaces or classes, I think we need a 
more generic mechanism for you to express that.


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Ping!

This has been stale for a while now. Last comment indicate this is not the 
right approach, but some prototyping has been done a more generic approach, 
with no timeline.
I understand this, but in the meantime the problem is still there, with no 
solution at the moment...

Can we move this forward in the mean time, possibly marking the API as 'beta', 
'unstable' or another way of saying it is subject to change. It can be removed 
when there is a better solution.
Or maybe there is now a timeline for the better solution? or the prototype is 
available somewhere, with some documentation on limits/works to be done, so 
that work can continue from there?


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 158309.
Typz added a comment.
Herald added a subscriber: acoomans.

Regeneration documentation


Repository:
  rC Clang

https://reviews.llvm.org/D37813

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 147027.
Typz added a comment.

Rebase


Repository:
  rC Clang

https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"} /* 

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D37813#958222, @klimek wrote:

> Some initial design work has been done, and Krasimir said that he's 
> interested. No timeline though :(


@klimek , @krasimir : any progress on this new preprocessor-like way to 
configure macros ?


Repository:
  rC Clang

https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Some initial design work has been done, and Krasimir said that he's 
> interested. No timeline though :(

any update or progress maybe?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 132838.
Typz added a comment.

rebase


https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"} /* end of TESTSUITE(A) */",

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#958116, @Typz wrote:

> OK.
>
> So you mean a solution like the one discussed earlier would be the way to go?
>
> > I mean that we can configure macros in the format style, like "define A(X) 
> > class X {". I'm not 100% sure whether we would just try to use the 
> > Preprocessor for this, or whether we'd want to only allow a small subset of 
> > actual macros, but the general idea would be the same: The 
> > UnwrappedLineParser would parse the macro at the expansion location A(X) 
> > into an unwrapped line, and then parse the expansion into a child line, 
> > with the tokens tha tare not in the argument of the call being marked as 
> > fixed (parent child might also be better inverted).
>
> (As a side-note, I want to stress out that we would actually need a 
> 'reversible' description to support the namespace case, to allow generating 
> the end-of-namespace comment)


I believe this will be needed for various reasons. The plan is to make this 
similar to how we format #ifdef trees, for which clang-format already has full 
support.

> Is there any work on that side, any timeline when this may be supported ?

Some initial design work has been done, and Krasimir said that he's interested. 
No timeline though :(


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

OK.

So you mean a solution like the one discussed earlier would be the way to go?

> I mean that we can configure macros in the format style, like "define A(X) 
> class X {". I'm not 100% sure whether we would just try to use the 
> Preprocessor for this, or whether we'd want to only allow a small subset of 
> actual macros, but the general idea would be the same: The 
> UnwrappedLineParser would parse the macro at the expansion location A(X) into 
> an unwrapped line, and then parse the expansion into a child line, with the 
> tokens tha tare not in the argument of the call being marked as fixed (parent 
> child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 
'reversible' description to support the namespace case, to allow generating the 
end-of-namespace comment)

Is there any work on that side, any timeline when this may be supported ?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

I've talked with Daniel and we both believe this patch is not the right way to 
go forward for clang-format. It is adding configuration mechanisms that we need 
to maintain going forward, without addressing a general problem; specifically 
for how to handle macros, I believe there is a clear better way to do it that 
is also fundamentally more useful than adding extra knobs for every problem we 
encounter. Finally, these changes do not address a fundamental bug, and 
workarounds exist, so there is no large benefit to do this now in a 
non-principled way rather than wait for a more principled fix. Overall, I do 
understand that this is a trade-off, but I hope you understand that we 
sometimes have to make decisions.


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

> Ok, that's probably where our different opinions come from - I would want a 
> macro to be formatted to reflect how it's implemented, because otherwise I'm 
> going to be surprised when I look at the implementation, and I consider 
> surprises to be something to avoid in programming in general, where possible.

As "author of the coding rules" (and thus writer of clang-format config), I 
would agree with you.
But I think the tool itself should not decide to enforce this : maybe there are 
cases where the macro is "conceptually" a namespace, but should be indented as 
a class ; in this case I think the tool should quite dumb, and simply do what 
was asked: if asked to format the macro as a namespace, it should do it, 
whatever the implementation is.

That said, on top of this "user choice" the tool could be smart, and 
automatically handle all macros based on their actual implementation: if the 
macro is actually implemented with a namespace (and there is no "explicit" 
definition of how it should be handled), then indeed the tool could format the 
whole block as a namespace, and so on... But unfortunately that would imply 
parsing numerous #include to get the definitions, and is definitely not the 
goal of this patch: this patch is really about letting the "user" decide how to 
handle the macro.

> That said, I'm also then a bit confused by your tests - they seem to 
> currently format a mixture of namespace and class formatting, and combine the 
> indentation of a class-scope with namespace end comments.

This should not be the case: the goal here is that the macro is handled exactly 
like a namespace.
The NamespaceEndCommentFixer tests indeed contain some indentation, but it is 
not significant and is there only because I copied existing test cases and 
followed the same "style": but these tests do not actually perform any 
indentation, they simply handle the "end comment".
Do you have any specific test which confuses you?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#945125, @Typz wrote:

> I don't think this is really relevant for this tool: if someone changes the 
> implementation of the macro, then *they* must indeed if it should not be 
> formatted like a namespace (and keep the clang-format configuration 
> unchanged), or if it should now be formatted like a class (and thus make 
> changes to clang-format configuration). Here we are not defining what the 
> macro does, but how clang-format should indent it : in most case I don't 
> think the indentation format should actually depend on the way it is 
> implemented...


Ok, that's probably where our different opinions come from - I would want a 
macro to be formatted to reflect how it's implemented, because otherwise I'm 
going to be surprised when I look at the implementation, and I consider 
surprises to be something to avoid in programming in general, where possible.

That said, I'm also then a bit confused by your tests - they seem to currently 
format a mixture of namespace and class formatting, and combine the indentation 
of a class-scope with namespace end comments.


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I don't think this is really relevant for this tool: if someone changes the 
implementation of the macro, then *they* must indeed if it should not be 
formatted like a namespace (and keep the clang-format configuration unchanged), 
or if it should now be formatted like a class (and thus make changes to 
clang-format configuration). Here we are not defining what the macro does, but 
how clang-format should indent it : in most case I don't think the indentation 
format should actually depend on the way it is implemented...


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#942137, @Typz wrote:

> As far as "parsing" and formatting inside the block is concerned, this is 
> indeed unrelated (and would totally work if all macros where specified with 
> some preprocessor definitions).
>
> But identifying the 'opening' token and generating the matching 'closing' 
> comment are totally related; it would seem very strange to have an option to 
> specify that TESTSUITE() macros are parsed as namespace, then another option 
> to indicate that namespace declared by this macros are actually closed with 
> another macro call...


Putting the namespace into the macro gives the macro an abstraction - if 
somebody were to change, for example, the namespace portion with a class 
definition, would you still want the closing comments or not?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

As far as "parsing" and formatting inside the block is concerned, this is 
indeed unrelated (and would totally work if all macros where specified with 
some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' 
comment are totally related; it would seem very strange to have an option to 
specify that TESTSUITE() macros are parsed as namespace, then another option to 
indicate that namespace declared by this macros are actually closed with 
another macro call...


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#941987, @Typz wrote:

> Definitely that would be much more expressive. But this approach is also 
> incomplete: in case of namespace (and possibly others?), we also need to 
> perform the reverse operation, e.g. to "generate" a macro call for rewriting 
> the closing comment.
>
> On top of this, I think that is really not trivial, I would prefer to move 
> forward with these "simpler" patch, and change the whole macro configurations 
> part in later commits...


Looking at the tests, I see what you mean, but I'd actually vote to not add 
non-namespace closing comments, or at least make closing comments configurable 
in a different way - it seems very much unrelated?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

Definitely that would be much more expressive. But this approach is also 
incomplete: in case of namespace (and possibly others?), we also need to 
perform the reverse operation, e.g. to "generate" a macro call for rewriting 
the closing comment.

On top of this, I think that is really not trivial, I would prefer to move 
forward with these "simpler" patch, and change the whole macro configurations 
part in later commits...


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D37813#930065, @Typz wrote:

> ping?


Argh, very sorry for the delay in response.

In https://reviews.llvm.org/D37813#905257, @Typz wrote:

> In https://reviews.llvm.org/D37813#876227, @klimek wrote:
>
> > I think instead of introducing more and more special cases of macros we 
> > might want to handle, we should instead allow specifying macro productions 
> > globally.
>
>
> what do you mean?
>  Do you mean to group all the macro configuration options into "Macros" 
> field, still containing one field for each kind of macro? Or do you have 
> something else in mind?


I mean that we can configure macros in the format style, like "define A(X) 
class X {". I'm not 100% sure whether we would just try to use the Preprocessor 
for this, or whether we'd want to only allow a small subset of actual macros, 
but the general idea would be the same: The UnwrappedLineParser would parse the 
macro at the expansion location A(X) into an unwrapped line, and then parse the 
expansion into a child line, with the tokens tha tare not in the argument of 
the call being marked as fixed (parent child might also be better inverted).

That will allow folks to actually specify the semantics they care about instead 
of us growing ever increasing special-case logic for different types of macros.


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 120085.
Typz added a comment.

rebase


https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"} /* end of TESTSUITE(A) */",

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D37813#876227, @klimek wrote:

> I think instead of introducing more and more special cases of macros we might 
> want to handle, we should instead allow specifying macro productions globally.


what do you mean?
Do you mean to group all the macro configuration options into "Macros" field, 
still containing one field for each kind of macro? Or do you have something 
else in mind?


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

I think instead of introducing more and more special cases of macros we might 
want to handle, we should instead allow specifying macro productions globally.


https://reviews.llvm.org/D37813



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

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 115802.
Typz added a comment.

rebase


https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"} // end anonymous TESTSUITE()",
+Style));
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"} /* end of TESTSUITE(A) */",

[PATCH] D37813: clang-format: better handle namespace macros

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.

Other macros are used to declare namespaces, and should thus be handled
similarly. This is the case for crpcut's TESTSUITE macro, or for
unittest-cpp's SUITE macro:

  TESTSUITE(Foo) {
  TEST(MyFirstTest) {
assert(0);
  }
  } // TESTSUITE(Foo)

This patch deals with this cases by introducing a new option to specify
lists of namespace macros. Internally, it re-uses the system already in
place for foreach and statement macros, to ensure there is no impact on
performance.


https://reviews.llvm.org/D37813

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/NamespaceEndCommentsFixer.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/NamespaceEndCommentsFixerTest.cpp

Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -53,6 +53,7 @@
 "  int i;\n"
 "  int j;\n"
 "}"));
+
   EXPECT_EQ("namespace {\n"
 "  int i;\n"
 "  int j;\n"
@@ -249,6 +250,85 @@
 "// unrelated"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, AddsMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+  Style.NamespaceMacros.push_back("TESTSUITE");
+
+  EXPECT_EQ("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE()",
+fixNamespaceEndComments("TESTSUITE() {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+
+  EXPECT_EQ("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("inline TESTSUITE(A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A)",
+fixNamespaceEndComments("TESTSUITE(::A) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(::A::B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(::A::B)",
+fixNamespaceEndComments("TESTSUITE(/**/::/**/A/**/::/**/B/**/) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(A)",
+fixNamespaceEndComments("TESTSUITE(A, B) {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+  EXPECT_EQ("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}// TESTSUITE(\"Test1\")",
+fixNamespaceEndComments("TESTSUITE(\"Test1\") {\n"
+"  int i;\n"
+"  int j;\n"
+"}",
+Style));
+}
+
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
 "  int i;\n"
@@ -381,6 +461,54 @@
 "}; /* unnamed namespace */"));
 }
 
+TEST_F(NamespaceEndCommentsFixerTest, KeepsValidMacroEndComment) {
+  FormatStyle Style = getLLVMStyle();
+