clang-format: Fix formatting C++ namespaces with preceding 'inline' or 'export' specifier

[melver]

Summary

This fixes formatting namespaces with preceding 'inline' and 'export' (Modules TS) specifiers.

This change fixes namespaces not being identified as such with preceding 'inline' or 'export' specifiers.

Motivation: I was experimenting with the Modules TS (-fmodules-ts) and found it would be useful if clang-format would correctly format 'export namespace'. While making the changes, I noticed that similar issues still exist with 'inline namespace', and addressed them as well.

[owenpan]

By the way, I didn't review the test cases.

lib/Format/FormatToken.h

524–525 ↗

(On Diff #161561)

if (NamespaceTok && NamespaceTok->isOneOf(tok::kw_inline, tok::kw_export))

lib/Format/NamespaceEndCommentsFixer.cpp

128–133 ↗

(On Diff #161561)

These lines are functionally the same as lines 523-528 in FormatToken.h. Refactor them?

129–130 ↗

(On Diff #161561)

if (NamespaceTok && NamespaceTok->isOneOf(tok::kw_inline, tok::kw_export))

lib/Format/UnwrappedLineParser.cpp

992–998 ↗

(On Diff #161561)

Move this case to after the case tok::kw_export below.

1066–1072 ↗

(On Diff #161561)

  if (!Style.isCpp())
    break;
case tok::kw_inline:
  nextToken();
  if (FormatTok->Tok.is(tok::kw_namespace)) {
    parseNamespace();
    return;
  }

[owenpan]

lib/Format/NamespaceEndCommentsFixer.cpp

129–130 ↗

(On Diff #161561)

Actually, I meant it for line 129 only.

[owenpan]

lib/Format/NamespaceEndCommentsFixer.cpp

128–133 ↗

(On Diff #161561)

I think this is better:

return NamespaceTok->getNamespaceToken();

[owenpan]

lib/Format/UnwrappedLineParser.cpp

1066–1072 ↗

(On Diff #161561)

I forgot to include LLVM_FALLTHROUGH to suppress the warning:

  if (!Style.isCpp())
    break;
  LLVM_FALLTHROUGH;
case tok::kw_inline:
  nextToken();
  if (FormatTok->Tok.is(tok::kw_namespace)) {
    parseNamespace();
    return;
  }

[melver]

Many thanks for the suggestions!

PTAL.

[sammccall]

Nice fix!

lib/Format/Format.cpp

1312 ↗

(On Diff #161960)

these 3 startswith checks appear 4 times now, you could pull out a helper function startsWithNamespace in FormatToken.h or something like that.
Up to you.

[sammccall]

unittests/Format/FormatTest.cpp

7582 ↗

(On Diff #161960)

you may want to add tests for other modules TS syntax (e.g. non-namespace export decls).
It seems this works well today, but without tests it could regress.

[owenpan]

lib/Format/Format.cpp

1312 ↗

(On Diff #161960)

I missed it. Good catch!

[owenpan]

lib/Format/UnwrappedLineFormatter.cpp

533–535 ↗

(On Diff #161960)

Maybe add a test case (or modify an existing one) for this fix, with a C/C++ style comment before namespace, inline, and/or export?

[melver]

Many thanks! PTAL.

[melver]

lib/Format/Format.cpp

1312 ↗

(On Diff #161960)

Added startsWithNamespace to TokenAnnotator.h.

unittests/Format/FormatTest.cpp

7582 ↗

(On Diff #161960)

I've added a couple for 'export class' with access specifiers. Otherwise, any other additions should probably be in future non-namespace related patches.

[melver]

Awaiting remaining reviewer acceptance.

FYI: I do not have commit commit access -- what is the procedure to commit once diff is accepted?

Many thanks!

[sammccall]

In D51036#1223230, @melver wrote:

Awaiting remaining reviewer acceptance.

FYI: I do not have commit commit access -- what is the procedure to commit once diff is accepted?

Many thanks!

Anyone with commit access can land it for you - I'm happy to do this.
@owenpan any concerns?

[owenpan]

In D51036#1223254, @sammccall wrote:

In D51036#1223230, @melver wrote:

Awaiting remaining reviewer acceptance.

FYI: I do not have commit commit access -- what is the procedure to commit once diff is accepted?

Many thanks!

Anyone with commit access can land it for you - I'm happy to do this.
@owenpan any concerns?

@sammccall Go ahead! :)

[melver]

In D51036#1223254, @sammccall wrote:

In D51036#1223230, @melver wrote:

Awaiting remaining reviewer acceptance.

FYI: I do not have commit commit access -- what is the procedure to commit once diff is accepted?

Many thanks!

Anyone with commit access can land it for you - I'm happy to do this.
@owenpan any concerns?

Great, many thanks for committing.

cfe/trunk/lib/Format/Format.cpp

bool containsOnlyComments(const AnnotatedLine &Line) {
     return true;
   }
 
   // Iterate through all lines and remove any empty (nested) namespaces.
   void checkEmptyNamespace(SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
     std::set<unsigned> DeletedLines;
     for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
       auto &Line = *AnnotatedLines[i];
-      if (Line.startsWith(tok::kw_namespace) ||
-          Line.startsWith(tok::kw_inline, tok::kw_namespace)) {
+      if (Line.startsWithNamespace()) {
         checkEmptyNamespace(AnnotatedLines, i, i, DeletedLines);
       }
     }
 
     for (auto Line : DeletedLines) {
       FormatToken *Tok = AnnotatedLines[Line]->First;
       while (Tok) {
         deleteToken(Tok);
if (Style.BraceWrapping.AfterNamespace) {
       }
     } else if (!AnnotatedLines[CurrentLine]->endsWith(tok::l_brace)) {
       return false;
     }
     while (++CurrentLine < End) {
       if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
         break;
 
-      if (AnnotatedLines[CurrentLine]->startsWith(tok::kw_namespace) ||
-          AnnotatedLines[CurrentLine]->startsWith(tok::kw_inline,
-                                                  tok::kw_namespace)) {
+      if (AnnotatedLines[CurrentLine]->startsWithNamespace()) {
         if (!checkEmptyNamespace(AnnotatedLines, CurrentLine, NewLine,
                                  DeletedLines))
           return false;
         CurrentLine = NewLine;
         continue;
       }
 
       if (containsOnlyComments(*AnnotatedLines[CurrentLine]))

cfe/trunk/lib/Format/FormatToken.h

struct FormatToken {
   }
 
   /// Return the actual namespace token, if this token starts a namespace
   /// block.
   const FormatToken *getNamespaceToken() const {
     const FormatToken *NamespaceTok = this;
     if (is(tok::comment))
       NamespaceTok = NamespaceTok->getNextNonComment();
-    // Detect "(inline)? namespace" in the beginning of a line.
-    if (NamespaceTok && NamespaceTok->is(tok::kw_inline))
+    // Detect "(inline|export)? namespace" in the beginning of a line.
+    if (NamespaceTok && NamespaceTok->isOneOf(tok::kw_inline, tok::kw_export))
       NamespaceTok = NamespaceTok->getNextNonComment();
     return NamespaceTok && NamespaceTok->is(tok::kw_namespace) ? NamespaceTok
                                                                : nullptr;
   }
 
 private:
   // Disallow copying.
   FormatToken(const FormatToken &) = delete;

cfe/trunk/lib/Format/NamespaceEndCommentsFixer.cpp

getNamespaceToken(const AnnotatedLine *Line,
   assert(StartLineIndex < AnnotatedLines.size());
   const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
   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)
       NamespaceTok = AnnotatedLines[StartLineIndex - 1]->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();
 }
 
 NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
                                                      const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 std::pair<tooling::Replacements, unsigned> NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,

cfe/trunk/lib/Format/TokenAnnotator.h

public:
   /// function declaration. Asserts MightBeFunctionDecl.
   bool mightBeFunctionDefinition() const {
     assert(MightBeFunctionDecl);
     // FIXME: Line.Last points to other characters than tok::semi
     // and tok::lbrace.
     return !Last->isOneOf(tok::semi, tok::comment);
   }
 
+  /// \c true if this line starts a namespace definition.
+  bool startsWithNamespace() const {
+    return startsWith(tok::kw_namespace) ||
+           startsWith(tok::kw_inline, tok::kw_namespace) ||
+           startsWith(tok::kw_export, tok::kw_namespace);
+  }
+
   FormatToken *First;
   FormatToken *Last;
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
   size_t MatchingOpeningBlockLineIndex;

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

if (Line.Last->is(tok::l_brace)) {
       FormatToken *Tok = I[1]->First;
       if (Tok->is(tok::r_brace) && !Tok->MustBreakBefore &&
           (Tok->getNextNonComment() == nullptr ||
            Tok->getNextNonComment()->is(tok::semi))) {
         // We merge empty blocks even if the line exceeds the column limit.
         Tok->SpacesRequiredBefore = 0;
         Tok->CanBreakBefore = true;
         return 1;
-      } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
+      } else if (Limit != 0 && !Line.startsWithNamespace() &&
                  !startsExternCBlock(Line)) {
         // We don't merge short records.
         FormatToken *RecordTok = Line.First;
         // Skip record modifiers.
         while (RecordTok->Next &&
                RecordTok->isOneOf(tok::kw_typedef, tok::kw_export,
                                   Keywords.kw_declare, Keywords.kw_abstract,
                                   tok::kw_default))
void UnwrappedLineFormatter::formatFirstToken(
   if (Newlines == 0 && !RootToken.IsFirst)
     Newlines = 1;
   if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
     Newlines = 0;
 
   // Remove empty lines after "{".
   if (!Style.KeepEmptyLinesAtTheStartOfBlocks && PreviousLine &&
       PreviousLine->Last->is(tok::l_brace) &&
-      PreviousLine->First->isNot(tok::kw_namespace) &&
+      !PreviousLine->startsWithNamespace() &&
       !startsExternCBlock(*PreviousLine))
     Newlines = 1;
 
   // Insert extra new line before access specifiers.
   if (PreviousLine && PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
       RootToken.isAccessSpecifier() && RootToken.NewlinesBefore == 1)
     ++Newlines;
 

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

if (FormatTok->is(tok::l_brace)) {
         FormatTok->Finalized = true;
         nextToken();
       }
     }
     break;
   case tok::kw_namespace:
     parseNamespace();
     return;
-  case tok::kw_inline:
-    nextToken();
-    if (FormatTok->Tok.is(tok::kw_namespace)) {
-      parseNamespace();
-      return;
-    }
-    break;
   case tok::kw_public:
   case tok::kw_protected:
   case tok::kw_private:
     if (Style.Language == FormatStyle::LK_Java ||
         Style.Language == FormatStyle::LK_JavaScript)
       nextToken();
     else
       parseAccessSpecifier();
if (FormatTok->Tok.is(tok::string_literal)) {
       }
     }
     break;
   case tok::kw_export:
     if (Style.Language == FormatStyle::LK_JavaScript) {
       parseJavaScriptEs6ImportExport();
       return;
     }
+    if (!Style.isCpp())
+      break;
+    // Handle C++ "(inline|export) namespace".
+    LLVM_FALLTHROUGH;
+  case tok::kw_inline:
+    nextToken();
+    if (FormatTok->Tok.is(tok::kw_namespace)) {
+      parseNamespace();
+      return;
+    }
     break;
   case tok::identifier:
     if (FormatTok->is(TT_ForEachMacro)) {
       parseForOrWhileLoop();
       return;
     }
     if (FormatTok->is(TT_MacroBlockBegin)) {
       parseBlock(/*MustBeDeclaration=*/false, /*AddLevel=*/true,

cfe/trunk/unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace N {\n"
             "\n"
             "int i;\n"
             "}",
             format("namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
+  EXPECT_EQ("/* something */ namespace N {\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("/* something */ namespace N {\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   getGoogleStyle()));
+  EXPECT_EQ("inline namespace N {\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("inline namespace N {\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   getGoogleStyle()));
+  EXPECT_EQ("/* something */ inline namespace N {\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("/* something */ inline namespace N {\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   getGoogleStyle()));
+  EXPECT_EQ("export namespace N {\n"
+            "\n"
+            "int i;\n"
+            "}",
+            format("export namespace N {\n"
+                   "\n"
+                   "int    i;\n"
+                   "}",
+                   getGoogleStyle()));
   EXPECT_EQ("extern /**/ \"C\" /**/ {\n"
             "\n"
             "int i;\n"
             "}",
             format("extern /**/ \"C\" /**/ {\n"
                    "\n"
                    "int    i;\n"
                    "}",

 TEST_F(FormatTest, UnderstandsAccessSpecifiers) {
   verifyFormat("class A {\n"
                "public:\n"
                "public: // comment\n"
                "protected:\n"
                "private:\n"
                "  void f() {}\n"
                "};");
+  verifyFormat("export class A {\n"
+               "public:\n"
+               "public: // comment\n"
+               "protected:\n"
+               "private:\n"
+               "  void f() {}\n"
+               "};");
   verifyGoogleFormat("class A {\n"
                      " public:\n"
                      " protected:\n"
                      " private:\n"
                      "  void f() {}\n"
                      "};");
+  verifyGoogleFormat("export class A {\n"
+                     " public:\n"
+                     " protected:\n"
+                     " private:\n"
+                     "  void f() {}\n"
+                     "};");
   verifyFormat("class A {\n"
                "public slots:\n"
                "  void f1() {}\n"
                "public Q_SLOTS:\n"
                "  void f2() {}\n"
                "protected slots:\n"
                "  void f3() {}\n"
                "protected Q_SLOTS:\n"
TEST_F(FormatTest, FormatsNamespaces) {
   FormatStyle LLVMWithNoNamespaceFix = getLLVMStyle();
   LLVMWithNoNamespaceFix.FixNamespaceComments = false;
 
   verifyFormat("namespace some_namespace {\n"
                "class A {};\n"
                "void f() { f(); }\n"
                "}",
                LLVMWithNoNamespaceFix);
+  verifyFormat("/* something */ namespace some_namespace {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
   verifyFormat("namespace {\n"
                "class A {};\n"
                "void f() { f(); }\n"
                "}",
                LLVMWithNoNamespaceFix);
+  verifyFormat("/* something */ namespace {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
   verifyFormat("inline namespace X {\n"
                "class A {};\n"
                "void f() { f(); }\n"
                "}",
                LLVMWithNoNamespaceFix);
+  verifyFormat("/* something */ inline namespace X {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
+  verifyFormat("export namespace X {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
   verifyFormat("using namespace some_namespace;\n"
                "class A {};\n"
                "void f() { f(); }",
                LLVMWithNoNamespaceFix);
 
   // This code is more common than we thought; if we
   // layout this correctly the semicolon will go into
   // its own line, which is undesirable.
verifyFormat("namespace Foo\n"
                "{};",
                Style);
   verifyFormat("/* something */ namespace Foo\n"
                "{};",
                Style);
   verifyFormat("inline namespace Foo\n"
                "{};",
                Style);
+  verifyFormat("/* something */ inline namespace Foo\n"
+               "{};",
+               Style);
+  verifyFormat("export namespace Foo\n"
+               "{};",
+               Style);
   verifyFormat("namespace Foo\n"
                "{\n"
                "void Bar();\n"
                "};",
                Style);
 }
 
 TEST_F(FormatTest, NeverMergeShortRecords) {