clang-format: Add CompactNamespaces option

[Typz]

Summary

Add CompactNamespaces option, to pack namespace declarations on the
same line (somewhat similar to C++17 nested namespace definition).

With this option, consecutive namespace declarations are kept on the
same line:

namespace foo { namespace bar {
    ...
}} // namespace foo::bar

[djasper]

For this and the other patches, could you please read: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
And make a statement as to whether these options qualify?

[Typz]

This option is used in multiple projects with public coding styles: OpenCV (http://code.opencv.org/projects/opencv/wiki/Coding_Style_Guide), Boost (http://www.boost.org/development/requirements.html)
And I am indeed willing to maintain the patch.

[Typz]

ping?

[kimgr]

We have a large closed-source codebase with this style, and would welcome clang-format support.

[djasper]

include/clang/Format/Format.h

153 ↗

(On Diff #96521)

While I am not entirely opposed to this feature, I think it should be a separate patch.

358 ↗

(On Diff #96521)

This is not bin packing at all. Maybe CompactNamespaces? Or SingleLineNamespaces?

Also, could you clarify what happens if the namespaces don't fit within the column limit (both in the comment describing the flag and by adding tests for it)?

[Typz]

include/clang/Format/Format.h

153 ↗

(On Diff #96521)

I totally agree, which is why I created 2 commits; however, since they modify the same code (and I am new to Phabricator) I could not find a way to upload them separately.

Is there a way? Or should I upload one, and upload the next one once the first one has been accepted?

[Typz]

clang-format: Add CompactNamespaces option

• Change option name to CompactNamespaces
• Clarify & test behavior when wrapping is needed
• Separate from the 'remove semicolon' patch

[Typz]

include/clang/Format/Format.h

153 ↗

(On Diff #96521)

OK, found the answer here: https://medium.com/@kurtisnusbaum/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6

358 ↗

(On Diff #96521)

This is not binpacking, but has a similar effect and made the option name somewhat consistent with the other binpack options :-)
I'll change to CompactNamespaces then.

[djasper]

include/clang/Format/Format.h

769 ↗

(On Diff #99421)

What happened here?

358 ↗

(On Diff #96521)

How does this option interact with NamespaceIndentation. Do all the values you can set there make sense and work as expected?

I am wondering whether we should generally rename this to NamespaceStyle and make it an enum. That way, we can start to also support C++17 namespace, but people that don't want to use C++17 yet, can still use this style of compact namespace.

lib/Format/NamespaceEndCommentsFixer.cpp

124 ↗

(On Diff #99421)

no space after "*"..

127 ↗

(On Diff #99421)

s/NULL/nullptr/

172 ↗

(On Diff #99421)

Do we need any of this if CompactNamespaces if false? Maybe we should surround the entire thing with an

if (Style.CompactNamespaces) {

lib/Format/UnwrappedLineFormatter.cpp

130 ↗

(On Diff #99421)

You always seem to call this function with Line->First. Maybe just call it on an AnnotatedLine?

134 ↗

(On Diff #99421)

just

return NamespaceTok && NamespaceTok->is(tok::kw_namespace);

139 ↗

(On Diff #99421)

s/line/Line/

221 ↗

(On Diff #99421)

This looks like you wanted to write a for loop.

223 ↗

(On Diff #99421)

Indentation

unittests/Format/FormatTest.cpp

1310 ↗

(On Diff #99421)

There need to be more tests. Specifically:

// Input is already in the desired format
namespace A { namespace B { .. }} // namespace A::B

// Input is contracted, but wrong comment
namespace A { namespace B {..}} // namespace A

// Input is partially contracted
namespace A { namespace B { namespace C {
}} // namespace B::C
} // namespace A

[Typz]

update for review comments

[Typz]

include/clang/Format/Format.h

358 ↗

(On Diff #96521)

How does this option interact with NamespaceIndentation. Do all the values you can set there make sense and work as expected?

NamespaceIndentation is not affected, indent is done as before (e.g. just "counting" the imbricated namespaces).

In 'NI_All' we may want to reduce the indent when multiple namespaces are declared in the same line, but this would become inconsistent if the beginning and end of all namespaces do not match:

namepace A { namespace B {
    int i;
} // namespace B
  int i;
} // namespace A

So I think reducing indent in that case should be (if ever needed) another value in NamespaceIndentation...

I am wondering whether we should generally rename this to NamespaceStyle and make it an enum. That way, we can start to also support C++17 namespace, but people that don't want to use C++17 yet, can still use this style of compact namespace.

As for C++17, I am not sure we need another option: just having CompactNamespaces=true and Standard=C++17 would use the "real" C++17 mode. That said converting to C++17 namespace blocks is slightly more restrictive, as it will require that both the beginning and end of the inner & outer blocks to match...

I will keep the boolean flag for now, just let me know if you prefer to have the enum in case other modes are needed and I will update the patch.

lib/Format/UnwrappedLineFormatter.cpp

139 ↗

(On Diff #99421)

these methods (isEndOfNamespace, ...) are somewhat duplicated in UnwrapperLinesFormatter and NamespaceEndComments (not really, but very similar) : should I factorize these into helper methods of FormatToken?

[djasper]

include/clang/Format/Format.h

358 ↗

(On Diff #96521)

Yeah, this is probably something nobody will ever want:

namepace A { namespace B {
    int i;
} // namespace B
  int i;
} // namespace A

And you have the same problem for NI_Inner as soon as you have more than two levels of namespace.

I see two ways forward:

1. Make "compacted" namespaces always add at most one level of indentation.
2. Assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

The problem I have with #1 is that it increases the complexity quite a bit and the behavior is even difficult to predict to users. Remove a comment somewhere might enable clang-format to make two namespaces "compact" and then remove indentation throughout the file.

So I would lean more towards solution #2. The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it. And of course I don't know whether some people would want indentation in compacted namespaces.

What do you think?

[Typz]

include/clang/Format/Format.h

358 ↗

(On Diff #96521)

I think current behavior is at least consistent (and simple to implement), and I agree that #2 would be the way to go to implement special nanespace indent rules for compacted namespaces.

Personnally, I don't need namespace indentation (we use NI_None), but i can add try to add an extra style NI_Compact to indent one level when in any number of namespaces (e.g. indentLevel = namespaceDepth>0).

That should probably be a separate patch however?

[djasper]

What I mean is that you should remove the CompactNamespace option and instead let this be configured by an additional enum value in NamespaceIndentation.

[Typz]

Merging the 2 options is definitely a "safe" option, as it prevents ensures only the most obvious behavior is accessible.

However, it has significant (IMO) drawbacks:

• "Compact" is a not an namespace indentation type, this will make the option quite confusing
• If indentation inside compact namespaces is needed, it cannot easily be added: we would need an extra mode NI_CompactWithIndent

All in all, I think I prefer the current behavior of the patch (a separate CompactNamespace options, with consistent [if not useful] indentation settings); but it is your call, just let me know how to proceed.

[Typz]

Remove dependency on D33314

[djasper]

That's what I meant by "The name NamespaceIndentation might then be a bit confusing, but not sure whether it's worth changing it.".

I am honestly not sure. Let's get a third opinion.

If we add this additional option, I think we need to fix the behavior and make what people expect:

With NI_None:

namespace A { namespace B {
int i;
namespace C {
int j;
} // namespace C
}} // namespace A::B

With NI_Inner:

namespace A { namespace B {
int i;
namespace C {
  int j;
} // namespace C
}} // namespace A::B

With NI_All:

namespace A { namespace B {
  int i;
  namespace C {
    int j;
  } // namespace C
}} // namespace A::B

[krasimir]

This change should also adapt NamespaceEndCommentFixer to respect the new option and not introduce/remove/change the comments unexpectedly.

[Typz]

Just to be clear: as it is, the patch does not implement what you describe, both on the 'indent' side (as expected, we are discussing it), but also on the "merging" of braces side. Consecutive namespace opening are merged on one side; and consecutive namespace closing are merged on the other side; but there is no relation between the two.

What this means is that at the moment your exemple would be formatted (without indent) like this:

namespace A { namespace B {
int i;
namespace C {
int j;
}}} // namespace A::B::C

[Typz]

In D32480#761859, @krasimir wrote:

This change should also adapt NamespaceEndCommentFixer to respect the new option and not introduce/remove/change the comments unexpectedly.

This should be handled already (pending further changes to the patch, obviously); did you see (or think of) any specific broken or dangerous case?

[krasimir]

In any case, adding a namespace end comment to a line closing multiple namespaces is super confusing for me: what does the comment refer to: the inner one, the outer one, or both?
}} // namespace A::B

[Typz]

In D32480#761862, @krasimir wrote:

In any case, adding a namespace end comment to a line closing multiple namespaces is super confusing for me: what does the comment refer to: the inner one, the outer one, or both?
}} // namespace A::B

it refers to both, similar to C++17' _nested namespace_ definition:

namespace A::B {
}} // namespace A::B

But the whole problem is indeed that we are trying to do something similar, but in a more generic situations: leading to either a complicated implementation to merge only in the same conditions as C++17 (e.g. both namespaces begin and end at the same place) or have to handle somewhat complicated scenarios, with the blocks being partially merged (as leading to complications in indentation...)

[Typz]

ping?

[klimek]

I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces).

For what it's worth, I'd

1. vote for only merging in cases where both start and end can be merged
2. vote for merge namespaces counting as a single namespace decl

[Typz]

That will be slightly more complicated to check, esp. we will need to keep track of the matching-closing-brace (currently only the matching-opening-brace) is stored.
But I can try to update the patch in that direction, if that is the consensus.

[Typz]

So how do I proceed?

1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation
2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

And should we limit 'compacting' to namespace which start and end at consecutive lines [e.g. exactly the same conditions as C++17 nested namespaces], or is the current implementation enough [merge all adjacent beginning and all adjacent endings] ?

[Typz]

ping?

[klimek]

In D32480#773807, @Typz wrote:

So how do I proceed?

1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation
2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact.

I'd vote for (1).

And should we limit 'compacting' to namespace which start and end at consecutive lines [e.g. exactly the same conditions as C++17 nested namespaces], or is the current implementation enough [merge all adjacent beginning and all adjacent endings] ?

I'd vote for the former.

[djasper]

Ok. Works for me.

[Typz]

• make "compacted" namespaces always add at most one level of indentation
• compact only namespaces which both start and end on consecutive lines

[klimek]

Generally LG from my side.

unittests/Format/FormatTest.cpp

1363–1381 ↗

(On Diff #102346)

These tests become more readable if you set up a style with a smaller column limit.

[djasper]

Yeah, looks good.

Krasimir, any further concerns?

[Typz]

Make tests more readable

[krasimir]

Looks good, except for the tests that actually add or fix namespace end comments should be moved to NamespaceEndCommentsFixerTest.cpp.

[krasimir]

unittests/Format/FormatTest.cpp

1394 ↗

(On Diff #102525)

Re-indent this line.

[Typz]

Move tests that add or fix namespace end comments to NamespaceEndCommentsFixerTest.cpp

cfe/trunk/include/clang/Format/Format.h

struct FormatStyle {
   ///    true:                                  false:
   ///    class MyClass                  vs.     class MyClass : public X, public Y {
   ///        : public X                         };
   ///        , public Y {
   ///    };
   /// \endcode
   bool BreakBeforeInheritanceComma;
 
+  /// \brief If ``true``, consecutive namespace declarations will be on the same
+  /// line. If ``false``, each namespace is declared on a new line.
+  /// \code
+  ///   true:
+  ///   namespace Foo { namespace Bar {
+  ///   }}
+  ///
+  ///   false:
+  ///   namespace Foo {
+  ///   namespace Bar {
+  ///   }
+  ///   }
+  /// \endcode
+  ///
+  /// If it does not fit on a single line, the overflowing namespaces get
+  /// wrapped:
+  /// \code
+  ///   namespace Foo { namespace Bar {
+  ///   namespace Extra {
+  ///   }}}
+  /// \endcode
+  bool CompactNamespaces;
+
   /// \brief If the constructor initializers don't fit on a line, put each
   /// initializer on its own line.
   /// \code
   ///   true:
   ///   SomeClass::Constructor()
   ///       : aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaa), aaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaa) {
   ///     return 0;
   ///   }
return AccessModifierOffset == R.AccessModifierOffset &&
            AlwaysBreakTemplateDeclarations ==
                R.AlwaysBreakTemplateDeclarations &&
            BinPackArguments == R.BinPackArguments &&
            BinPackParameters == R.BinPackParameters &&
            BreakBeforeBinaryOperators == R.BreakBeforeBinaryOperators &&
            BreakBeforeBraces == R.BreakBeforeBraces &&
            BreakBeforeTernaryOperators == R.BreakBeforeTernaryOperators &&
            BreakConstructorInitializers == R.BreakConstructorInitializers &&
+           CompactNamespaces == R.CompactNamespaces &&
            BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
            BreakStringLiterals == R.BreakStringLiterals &&
            ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
            BreakBeforeInheritanceComma == R.BreakBeforeInheritanceComma &&
            ConstructorInitializerAllOnOneLineOrOnePerLine ==
                R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&

cfe/trunk/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("AlwaysBreakTemplateDeclarations",
                    Style.AlwaysBreakTemplateDeclarations);
     IO.mapOptional("BinPackArguments", Style.BinPackArguments);
     IO.mapOptional("BinPackParameters", Style.BinPackParameters);
     IO.mapOptional("BraceWrapping", Style.BraceWrapping);
     IO.mapOptional("BreakBeforeBinaryOperators",
                    Style.BreakBeforeBinaryOperators);
     IO.mapOptional("BreakBeforeBraces", Style.BreakBeforeBraces);
+    IO.mapOptional("BreakBeforeInheritanceComma",
+                   Style.BreakBeforeInheritanceComma);
     IO.mapOptional("BreakBeforeTernaryOperators",
                    Style.BreakBeforeTernaryOperators);
 
     bool BreakConstructorInitializersBeforeComma = false;
     IO.mapOptional("BreakConstructorInitializersBeforeComma",
                    BreakConstructorInitializersBeforeComma);
     IO.mapOptional("BreakConstructorInitializers",
                    Style.BreakConstructorInitializers);
     // If BreakConstructorInitializersBeforeComma was specified but
     // BreakConstructorInitializers was not, initialize the latter from the
     // former for backwards compatibility.
     if (BreakConstructorInitializersBeforeComma &&
         Style.BreakConstructorInitializers == FormatStyle::BCIS_BeforeColon)
       Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
 
     IO.mapOptional("BreakAfterJavaFieldAnnotations",
                    Style.BreakAfterJavaFieldAnnotations);
     IO.mapOptional("BreakStringLiterals", Style.BreakStringLiterals);
     IO.mapOptional("ColumnLimit", Style.ColumnLimit);
     IO.mapOptional("CommentPragmas", Style.CommentPragmas);
-    IO.mapOptional("BreakBeforeInheritanceComma",
-                   Style.BreakBeforeInheritanceComma);
+    IO.mapOptional("CompactNamespaces", Style.CompactNamespaces);
     IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
                    Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
     IO.mapOptional("ConstructorInitializerIndentWidth",
                    Style.ConstructorInitializerIndentWidth);
     IO.mapOptional("ContinuationIndentWidth", Style.ContinuationIndentWidth);
     IO.mapOptional("Cpp11BracedListStyle", Style.Cpp11BracedListStyle);
     IO.mapOptional("DerivePointerAlignment", Style.DerivePointerAlignment);
     IO.mapOptional("DisableFormat", Style.DisableFormat);
FormatStyle getLLVMStyle() {
   LLVMStyle.AllowShortBlocksOnASingleLine = false;
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;
-  LLVMStyle.BinPackParameters = true;
   LLVMStyle.BinPackArguments = true;
+  LLVMStyle.BinPackParameters = true;
   LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
   LLVMStyle.BreakBeforeTernaryOperators = true;
   LLVMStyle.BreakBeforeBraces = FormatStyle::BS_Attach;
   LLVMStyle.BraceWrapping = {false, false, false, false, false, false,
                              false, false, false, false, false, true};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeColon;
   LLVMStyle.BreakBeforeInheritanceComma = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
+  LLVMStyle.CompactNamespaces = false;
   LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.ConstructorInitializerIndentWidth = 4;
   LLVMStyle.ContinuationIndentWidth = 4;
   LLVMStyle.Cpp11BracedListStyle = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");

cfe/trunk/lib/Format/NamespaceEndCommentsFixer.cpp

void updateEndComment(const FormatToken *RBraceTok, StringRef EndCommentText,
   auto Range = CharSourceRange::getCharRange(Comment->getStartOfNonWhitespace(),
                                              Comment->Tok.getEndLoc());
   auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, EndCommentText));
   if (Err) {
     llvm::errs() << "Error while updating namespace end comment: "
                  << llvm::toString(std::move(Err)) << "\n";
   }
 }
+
+const FormatToken *
+getNamespaceToken(const AnnotatedLine *line,
+                  const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
+  if (!line->Affected || line->InPPDirective || !line->startsWith(tok::r_brace))
+    return nullptr;
+  size_t StartLineIndex = line->MatchingOpeningBlockLineIndex;
+  if (StartLineIndex == UnwrappedLine::kInvalidIndex)
+    return nullptr;
+  assert(StartLineIndex < AnnotatedLines.size());
+  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;
+}
 } // namespace
 
 NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
                                                      const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 tooling::Replacements NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
   const SourceManager &SourceMgr = Env.getSourceManager();
   AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
                                         AnnotatedLines.end());
   tooling::Replacements Fixes;
+  std::string AllNamespaceNames = "";
+  size_t StartLineIndex = SIZE_MAX;
+  unsigned int CompactedNamespacesCount = 0;
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
-    if (!AnnotatedLines[I]->Affected || AnnotatedLines[I]->InPPDirective ||
-        !AnnotatedLines[I]->startsWith(tok::r_brace))
-      continue;
     const AnnotatedLine *EndLine = AnnotatedLines[I];
-    size_t StartLineIndex = EndLine->MatchingOpeningBlockLineIndex;
-    if (StartLineIndex == UnwrappedLine::kInvalidIndex)
-      continue;
-    assert(StartLineIndex < E);
-    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))
+    const FormatToken *NamespaceTok =
+        getNamespaceToken(EndLine, AnnotatedLines);
+    if (!NamespaceTok)
       continue;
     FormatToken *RBraceTok = EndLine->First;
     if (RBraceTok->Finalized)
       continue;
     RBraceTok->Finalized = true;
     const FormatToken *EndCommentPrevTok = RBraceTok;
     // Namespaces often end with '};'. In that case, attach namespace end
     // comments to the semicolon tokens.
     if (RBraceTok->Next && RBraceTok->Next->is(tok::semi)) {
       EndCommentPrevTok = RBraceTok->Next;
     }
+    if (StartLineIndex == SIZE_MAX)
+      StartLineIndex = EndLine->MatchingOpeningBlockLineIndex;
+    std::string NamespaceName = computeName(NamespaceTok);
+    if (Style.CompactNamespaces) {
+      if ((I + 1 < E) &&
+          getNamespaceToken(AnnotatedLines[I + 1], AnnotatedLines) &&
+          StartLineIndex - CompactedNamespacesCount - 1 ==
+              AnnotatedLines[I + 1]->MatchingOpeningBlockLineIndex &&
+          !AnnotatedLines[I + 1]->First->Finalized) {
+        if (hasEndComment(EndCommentPrevTok)) {
+          // remove end comment, it will be merged in next one
+          updateEndComment(EndCommentPrevTok, std::string(), SourceMgr, &Fixes);
+        }
+        CompactedNamespacesCount++;
+        AllNamespaceNames = "::" + NamespaceName + AllNamespaceNames;
+        continue;
+      }
+      NamespaceName += std::move(AllNamespaceNames);
+      CompactedNamespacesCount = 0;
+      AllNamespaceNames = std::string();
+    }
     // The next token in the token stream after the place where the end comment
     // token must be. This is either the next token on the current line or the
     // first token on the next line.
     const FormatToken *EndCommentNextTok = EndCommentPrevTok->Next;
     if (EndCommentNextTok && EndCommentNextTok->is(tok::comment))
       EndCommentNextTok = EndCommentNextTok->Next;
     if (!EndCommentNextTok && I + 1 < E)
       EndCommentNextTok = AnnotatedLines[I + 1]->First;
     bool AddNewline = EndCommentNextTok &&
                       EndCommentNextTok->NewlinesBefore == 0 &&
                       EndCommentNextTok->isNot(tok::eof);
-    const std::string NamespaceName = computeName(NamespaceTok);
     const std::string EndCommentText =
         computeEndCommentText(NamespaceName, AddNewline);
     if (!hasEndComment(EndCommentPrevTok)) {
       bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
       if (!isShort)
         addEndComment(EndCommentPrevTok, EndCommentText, SourceMgr, &Fixes);
-      continue;
-    }
-    if (!validEndComment(EndCommentPrevTok, NamespaceName))
+    } else if (!validEndComment(EndCommentPrevTok, NamespaceName)) {
       updateEndComment(EndCommentPrevTok, EndCommentText, SourceMgr, &Fixes);
     }
+    StartLineIndex = SIZE_MAX;
+  }
   return Fixes;
 }
 
 } // namespace format
 } // namespace clang

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

void nextLine(const AnnotatedLine &Line) {
     } else {
       IndentForLevel.resize(Line.Level + 1);
       Indent = getIndent(IndentForLevel, Line.Level);
     }
     if (static_cast<int>(Indent) + Offset >= 0)
       Indent += Offset;
   }
 
+  /// \brief Update the indent state given that \p Line indent should be
+  /// skipped.
+  void skipLine(const AnnotatedLine &Line) {
+    while (IndentForLevel.size() <= Line.Level)
+      IndentForLevel.push_back(Indent);
+  }
+
   /// \brief Update the level indent to adapt to the given \p Line.
   ///
   /// When a line is not formatted, we move the subsequent lines on the same
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
     unsigned LevelIndent = Line.First->OriginalColumn;
     if (static_cast<int>(LevelIndent) - Offset >= 0)
private:
   /// For example, the 'public' keywords is often indented with a negative
   /// offset.
   int Offset = 0;
 
   /// \brief The current line's indent.
   unsigned Indent = 0;
 };
 
+bool isNamespaceDeclaration(const AnnotatedLine *Line) {
+  const FormatToken *NamespaceTok = Line->First;
+  // Detect "(inline)? namespace" in the beginning of a line.
+  if (NamespaceTok->is(tok::kw_inline))
+    NamespaceTok = NamespaceTok->getNextNonComment();
+  return NamespaceTok && NamespaceTok->is(tok::kw_namespace);
+}
+
+bool isEndOfNamespace(const AnnotatedLine *Line,
+                      const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines) {
+  if (!Line->startsWith(tok::r_brace))
+    return false;
+  size_t StartLineIndex = Line->MatchingOpeningBlockLineIndex;
+  if (StartLineIndex == UnwrappedLine::kInvalidIndex)
+    return false;
+  assert(StartLineIndex < AnnotatedLines.size());
+  return isNamespaceDeclaration(AnnotatedLines[StartLineIndex]);
+}
+
 class LineJoiner {
 public:
   LineJoiner(const FormatStyle &Style, const AdditionalKeywords &Keywords,
              const SmallVectorImpl<AnnotatedLine *> &Lines)
-      : Style(Style), Keywords(Keywords), End(Lines.end()),
-        Next(Lines.begin()) {}
+      : Style(Style), Keywords(Keywords), End(Lines.end()), Next(Lines.begin()),
+        AnnotatedLines(Lines) {}
 
   /// \brief Returns the next line, merging multiple lines into one if possible.
   const AnnotatedLine *getNextMergedLine(bool DryRun,
                                          LevelIndentTracker &IndentTracker) {
     if (Next == End)
       return nullptr;
     const AnnotatedLine *Current = *Next;
     IndentTracker.nextLine(*Current);
     unsigned MergedLines =
-        tryFitMultipleLinesInOne(IndentTracker.getIndent(), Next, End);
+        tryFitMultipleLinesInOne(IndentTracker, Next, End);
     if (MergedLines > 0 && Style.ColumnLimit == 0)
       // Disallow line merging if there is a break at the start of one of the
       // input lines.
       for (unsigned i = 0; i < MergedLines; ++i)
         if (Next[i + 1]->First->NewlinesBefore > 0)
           MergedLines = 0;
     if (!DryRun)
       for (unsigned i = 0; i < MergedLines; ++i)
         join(*Next[0], *Next[i + 1]);
     Next = Next + MergedLines + 1;
     return Current;
   }
 
 private:
   /// \brief Calculates how many lines can be merged into 1 starting at \p I.
   unsigned
-  tryFitMultipleLinesInOne(unsigned Indent,
+  tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
                            SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                            SmallVectorImpl<AnnotatedLine *>::const_iterator E) {
+    const unsigned Indent = IndentTracker.getIndent();
+
     // Can't join the last line with anything.
     if (I + 1 == E)
       return 0;
     // We can never merge stuff if there are trailing line comments.
     const AnnotatedLine *TheLine = *I;
     if (TheLine->Last->is(TT_LineComment))
       return 0;
     if (I[1]->Type == LT_Invalid || I[1]->First->MustBreakBefore)
tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
     // If necessary, change to something smarter.
     bool MergeShortFunctions =
         Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
         (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
          I[1]->First->is(tok::r_brace)) ||
         (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline &&
          TheLine->Level != 0);
 
+    if (Style.CompactNamespaces) {
+      if (isNamespaceDeclaration(TheLine)) {
+        int i = 0;
+        unsigned closingLine = TheLine->MatchingOpeningBlockLineIndex - 1;
+        for (; I + 1 + i != E && isNamespaceDeclaration(I[i + 1]) &&
+               closingLine == I[i + 1]->MatchingOpeningBlockLineIndex &&
+               I[i + 1]->Last->TotalLength < Limit;
+             i++, closingLine--) {
+          // No extra indent for compacted namespaces
+          IndentTracker.skipLine(*I[i + 1]);
+
+          Limit -= I[i + 1]->Last->TotalLength;
+        }
+        return i;
+      }
+
+      if (isEndOfNamespace(TheLine, AnnotatedLines)) {
+        int i = 0;
+        unsigned openingLine = TheLine->MatchingOpeningBlockLineIndex - 1;
+        for (; I + 1 + i != E && isEndOfNamespace(I[i + 1], AnnotatedLines) &&
+               openingLine == I[i + 1]->MatchingOpeningBlockLineIndex;
+             i++, openingLine--) {
+          // No space between consecutive braces
+          I[i + 1]->First->SpacesRequiredBefore = !I[i]->Last->is(tok::r_brace);
+
+          // Indent like the outer-most namespace
+          IndentTracker.nextLine(*I[i + 1]);
+        }
+        return i;
+      }
+    }
+
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
     if (TheLine->Last->is(tok::l_brace)) {
       return !Style.BraceWrapping.AfterFunction
                  ? tryMergeSimpleBlock(I, E, Limit)
                  : 0;
void join(AnnotatedLine &A, const AnnotatedLine &B) {
     }
   }
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   const SmallVectorImpl<AnnotatedLine *>::const_iterator End;
 
   SmallVectorImpl<AnnotatedLine *>::const_iterator Next;
+  const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines;
 };
 
 static void markFinalized(FormatToken *Tok) {
   for (; Tok; Tok = Tok->Next) {
     Tok->Finalized = true;
     for (AnnotatedLine *Child : Tok->Children)
       markFinalized(Child->First);
   }

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->Level = InitialLevel;
   Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
+  if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) {
+    // Update the opening line to add the forward reference as well
+    (*CurrentLines)[OpeningLineIndex].MatchingOpeningBlockLineIndex =
+            CurrentLines->size() - 1;
+  }
 }
 
 static bool isGoogScope(const UnwrappedLine &Line) {
   // FIXME: Closure-library specific stuff should not be hard-coded but be
   // configurable.
   if (Line.Tokens.size() < 4)
     return false;
   auto I = Line.Tokens.begin();

cfe/trunk/unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace out {\n"
                    "int i;\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace out",
                    Style));
 }
 
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+  FormatStyle Style = getLLVMStyle();
+  Style.CompactNamespaces = true;
+
+  verifyFormat("namespace A { namespace B {\n"
+         "}} // namespace A::B",
+         Style);
+
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   Style));
+
+  // Only namespaces which have both consecutive opening and end get compacted
+  EXPECT_EQ("namespace out {\n"
+            "namespace in1 {\n"
+            "} // namespace in1\n"
+            "namespace in2 {\n"
+            "} // namespace in2\n"
+            "} // namespace out",
+            format("namespace out {\n"
+                   "namespace in1 {\n"
+                   "} // namespace in1\n"
+                   "namespace in2 {\n"
+                   "} // namespace in2\n"
+                   "} // namespace out",
+                   Style));
+
+  EXPECT_EQ("namespace out {\n"
+            "int i;\n"
+            "namespace in {\n"
+            "int j;\n"
+            "} // namespace in\n"
+            "int k;\n"
+            "} // namespace out",
+            format("namespace out { int i;\n"
+                   "namespace in { int j; } // namespace in\n"
+                   "int k; } // namespace out",
+                   Style));
+
+  EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+            "}}} // namespace A::B::C\n",
+            format("namespace A { namespace B {\n"
+                   "namespace C {\n"
+                   "}} // namespace B::C\n"
+                   "} // namespace A\n",
+                   Style));
+
+  Style.ColumnLimit = 40;
+  EXPECT_EQ("namespace aaaaaaaaaa {\n"
+            "namespace bbbbbbbbbb {\n"
+            "}} // namespace aaaaaaaaaa::bbbbbbbbbb",
+            format("namespace aaaaaaaaaa {\n"
+                   "namespace bbbbbbbbbb {\n"
+                   "} // namespace bbbbbbbbbb\n"
+                   "} // namespace aaaaaaaaaa",
+                   Style));
+
+  EXPECT_EQ("namespace aaaaaa { namespace bbbbbb {\n"
+            "namespace cccccc {\n"
+            "}}} // namespace aaaaaa::bbbbbb::cccccc",
+            format("namespace aaaaaa {\n"
+                   "namespace bbbbbb {\n"
+                   "namespace cccccc {\n"
+                   "} // namespace cccccc\n"
+                   "} // namespace bbbbbb\n"
+                   "} // namespace aaaaaa",
+                   Style));
+  Style.ColumnLimit = 80;
+
+  // Extra semicolon after 'inner' closing brace prevents merging
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}; } // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "}; // namespace in\n"
+                   "} // namespace out",
+                   Style));
+
+  // Extra semicolon after 'outer' closing brace is conserved
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}}; // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "} // namespace in\n"
+                   "}; // namespace out",
+                   Style));
+
+  Style.NamespaceIndentation = FormatStyle::NI_All;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "  int i;\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "int i;\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   Style));
+  EXPECT_EQ("namespace out { namespace mid {\n"
+            "  namespace in {\n"
+            "    int j;\n"
+            "  } // namespace in\n"
+            "  int k;\n"
+            "}} // namespace out::mid",
+            format("namespace out { namespace mid {\n"
+                   "namespace in { int j; } // namespace in\n"
+                   "int k; }} // namespace out::mid",
+                   Style));
+
+  Style.NamespaceIndentation = FormatStyle::NI_Inner;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "  int i;\n"
+            "}} // namespace out::in",
+            format("namespace out {\n"
+                   "namespace in {\n"
+                   "int i;\n"
+                   "} // namespace in\n"
+                   "} // namespace out",
+                   Style));
+  EXPECT_EQ("namespace out { namespace mid { namespace in {\n"
+            "  int i;\n"
+            "}}} // namespace out::mid::in",
+            format("namespace out {\n"
+                   "namespace mid {\n"
+                   "namespace in {\n"
+                   "int i;\n"
+                   "} // namespace in\n"
+                   "} // namespace mid\n"
+                   "} // namespace out",
+                   Style));
+}
+
 TEST_F(FormatTest, FormatsExternC) { verifyFormat("extern \"C\" {\nint a;"); }
 
 TEST_F(FormatTest, FormatsInlineASM) {
   verifyFormat("asm(\"xyz\" : \"=a\"(a), \"=d\"(b) : \"a\"(data));");
   verifyFormat("asm(\"nop\" ::: \"memory\");");
   verifyFormat(
       "asm(\"movq\\t%%rbx, %%rsi\\n\\t\"\n"
       "    \"cpuid\\n\\t\"\n"
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
   CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
   CHECK_PARSE_BOOL(BinPackArguments);
   CHECK_PARSE_BOOL(BinPackParameters);
   CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
   CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
+  CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);

cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp

EXPECT_EQ("namespace A {\n"
                                     "namespace B {\n"
                                     "namespace C {\n"
                                     "namespace D {\n"
                                     "}\n"
                                     "}\n"
                                     "}\n"
                                     "}"));
 
+  // Add comment for namespaces which will be 'compacted'
+  FormatStyle CompactNamespacesStyle = getLLVMStyle();
+  CompactNamespacesStyle.CompactNamespaces = true;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "int i;\n"
+            "int j;\n"
+            "}}// namespace out::in",
+            fixNamespaceEndComments("namespace out { namespace in {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}}",
+                                    CompactNamespacesStyle));
+  EXPECT_EQ("namespace out {\n"
+            "namespace in {\n"
+            "int i;\n"
+            "int j;\n"
+            "}\n"
+            "}// namespace out::in",
+            fixNamespaceEndComments("namespace out {\n"
+                                    "namespace in {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}\n"
+                                    "}",
+                                    CompactNamespacesStyle));
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "int i;\n"
+            "int j;\n"
+            "};}// namespace out::in",
+            fixNamespaceEndComments("namespace out { namespace in {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "};}",
+                                    CompactNamespacesStyle));
+
   // Adds an end comment after a semicolon.
   EXPECT_EQ("namespace {\n"
             "  int i;\n"
             "  int j;\n"
             "};// namespace",
             fixNamespaceEndComments("namespace {\n"
                                     "  int i;\n"
                                     "  int j;\n"
EXPECT_EQ("namespace A {\n"
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"
                                     "}; // banamespace A"));
   // Updates invalid line comments even for short namespaces.
   EXPECT_EQ("namespace A {} // namespace A",
             fixNamespaceEndComments("namespace A {} // namespace"));
   EXPECT_EQ("namespace A {}; // namespace A",
             fixNamespaceEndComments("namespace A {}; // namespace"));
+
+  // Update invalid comments for compacted namespaces.
+  FormatStyle CompactNamespacesStyle = getLLVMStyle();
+  CompactNamespacesStyle.CompactNamespaces = true;
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            fixNamespaceEndComments("namespace out { namespace in {\n"
+                                    "}} // namespace out",
+                                    CompactNamespacesStyle));
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}} // namespace out::in",
+            fixNamespaceEndComments("namespace out { namespace in {\n"
+                                    "}} // namespace in",
+                                    CompactNamespacesStyle));
+  EXPECT_EQ("namespace out { namespace in {\n"
+            "}\n"
+            "} // namespace out::in",
+            fixNamespaceEndComments("namespace out { namespace in {\n"
+                                    "}// banamespace in\n"
+                                    "} // namespace out",
+                                    CompactNamespacesStyle));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, UpdatesInvalidEndBlockComment) {
   EXPECT_EQ("namespace {\n"
             "  int i;\n"
             "} // namespace",
             fixNamespaceEndComments("namespace {\n"
                                     "  int i;\n"