[PATCH] D33440: clang-format: better handle statement and namespace macros

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Patch looks good, but I also would like to see it splited. I would suggest to first get the statement macro part in, which requires less code. Then we can put the namespace macros on top of that. I really like the generality of this approach and would want to also add support for `class` macros eventually.



================
Comment at: lib/Format/FormatTokenLexer.cpp:641
+    auto it = std::find(Macros.begin(), Macros.end(),
+                        FormatTok->Tok.getIdentifierInfo());
     if (!(Tokens.size() > 0 && Tokens.back()->Tok.getIdentifierInfo() &&
----------------
Please move this inside the following `if` statement, so that we only perform the search when we see a `tok::pp_define`.


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:56
+    assert(Tok && Tok->is(tok::l_paren) && "expected an opening parenthesis");
+    Tok = Tok ? Tok->getNextNonComment() : nullptr;
+    while (Tok && !Tok->isOneOf(tok::r_paren, tok::comma)) {
----------------
I don't understand why you have a `Tok ? ...` after you `assert(Tok && ...)`?


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:82
+    text += ")";
+  // close brace
   if (AddNewline)
----------------
What does this comment refer to? If it's about the line above, consider moving it up.


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:100
+    StringRef NamespaceTokenText = Groups.size() > 4 ? Groups[4] : "";
+    // The name of the macro must be used
+    if (NamespaceTokenText != NamespaceTok->TokenText)
----------------
nit: end comment with `.`


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:105
+             !kNamespaceCommentPattern.match(Comment->TokenText, &Groups)) {
+    // Comment does not match regex
+    return false;
----------------
nit: end comment with `.`


================
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:155
   const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
-  // Detect "(inline)? namespace" in the beginning of a line.
-  if (NamespaceTok->is(tok::kw_inline))
-    NamespaceTok = NamespaceTok->getNextNonComment();
-  if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace))
-    return nullptr;
-  return NamespaceTok;
+  return NamespaceTok->getNamespaceToken();
+}
----------------
What happened to the old `// Detect "(inline)? namespace" in the beginning of a line.`


================
Comment at: unittests/Format/FormatTest.cpp:1588
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  verifyFormat("TESTSUITE(A) {\n"
+               "  int foo();\n"
----------------
Hm, what would happen if you have a namespace macro with two or more parameters?


https://reviews.llvm.org/D33440