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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:72
 
-    Tok = FirstNSTok;
-    while (Tok && !Tok->is(tok::l_brace)) {
+    bool IsPrevColoncolon = false;
+    bool HasColoncolon = false;
----------------
Nit: Personally, I'd put these bools further down, closer to the place where they are used and after AddMacro lambda.


================
Comment at: clang/lib/Format/NamespaceEndCommentsFixer.cpp:85
+      Tok = Tok->getNextNonComment();
+      for (int NestLevel = 1; Tok && NestLevel > 0;) {
+        if (Tok->is(tok::l_paren))
----------------
This loop seems very similar to the one that skips the attributes on lines 45-53, could you please refactor?
A lambda taking a pair of token kinds (l_paren/r_paren, l_square/r_square) should be fine.


================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:192
                 "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A M(x) {\n"
----------------
zequanwu wrote:
> 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.
In a different test case, could you add A to AttributeMacros and test that we skip it like other attributes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121269