[PATCH] D121269: [clang-format] Fix namespace format when the name is followed by a macro

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192
                 "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A M(x) {\n"
----------------
zequanwu wrote:
> curdeius wrote:
> > MyDeveloperDay wrote:
> > > Can you test the A B case? We can’t have a space right?
> > What's the rationale behind keeping `M(x)` in one case and not the other?
> > How can you decide?
> In the first one, we keep `M(x)` because we don't know that is `A` or `M(x)` the name. 
> In the second one, because we see a `::`, so we know that's the name and discard `M(x)`.
Added a test with `A B`. 
In that case, we will have `// namespace A B`, since we don't know which one is the real name. Either one could be a macro that expands to an attribute.


================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192-211
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A M(x) {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A M(x)",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace A M(x) {\n"
----------------
curdeius wrote:
> MyDeveloperDay wrote:
> > Can you test the A B case? We can’t have a space right?
> What's the rationale behind keeping `M(x)` in one case and not the other?
> How can you decide?
In the first one, we keep `M(x)` because we don't know that is `A` or `M(x)` the name. 
In the second one, because we see a `::`, so we know that's the name and discard `M(x)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269