[clang-format] Add a NamespaceEndCommentsFixer

[krasimir]

Summary

This patch adds a NamespaceEndCommentsFixer TokenAnalyzer for clang-format,
which fixes end namespace comments.
It currently supports inserting and updating existing wrong comments.

Example source:

namespace A {
int i;
}

namespace B {
int j;
} // namespace A

after formatting:

namespace A {
int i;
} // namespace A

namespace B {
int j;
} // namespace B

[krasimir]

• Reformatted unit tests

[djasper]

I started reviewing, but after a while it occurred to me that some of this might not be the right approach. We are essentially duplicating the parsing of namespaces, which is already done in UnwrappedLineParser. Lets instead just store more information in the AnnotatedLines (or FormatTokens) to carry the required information forward.

Specifically, I think we should do two things:

1. Change UnwrappedLineFormatter::parseNamespace() to carry more information of the parse namespace.
2. Change UnwrappedLineFormatter entirely to compute and store information about matched braces. This can (I think) be store in the AnnotatedLines as we have pairs of lines that belong together.

lib/Format/NamespaceEndCommentsFixer.cpp

38

Why do you match so precisely? Why not just return all the non-comment tokens between "namespace" and "{"? What should happen if a namespace declaration doesn't match?

110

I think you should remove SpaceBefore. clang-format will subsequently clean this up. Or, alternatively, you should use clang-format Style option SpacesBeforeTrailingComments.

111

I think it's more efficient to construct strings like this with llvm::Twine

116

No braces for single-statement ifs.

184

I think we should always update with a //-comment

197

I'd find the control flow easier to read as:

if (!hasEndComment()) {
  addEndComment(Fixes);
  return;
}
if (!validEndComment()) {
  updateEndComment(Fixes);
}

230

This needs the comment. Also, why "append" and why don't you return a vector of namespace?

232

Why unique_ptr<Namespace> instead of just Namespace?

[klimek]

lib/Format/NamespaceEndCommentsFixer.cpp

111

Generally, you don't want to use Twine outside of a single statement.
Twine a = Twine("a") + Twine("b");
is a use-after-free, because 'a' keeps a pointer to the temporaries.

[krasimir]

• Address review comments

[krasimir]

@djasper: I don't entirely understand:

2. Change UnwrappedLineFormatter entirely to compute and store information about matched braces. This can (I think) be store in the AnnotatedLines as we have pairs of lines that belong together.

Does that mean that an unwrapped line cannot contain more than one brace? I'm also not sure how we can test such a change.

[djasper]

An unwrapped line can contain many braces, but (I think/hope) only one that belongs to a non-nested block. So every unwrapped line that opens a namespace/class/function/enum/... should have exactly one counterpart that closes it again.

Now, there are nested blocks, e.g. in lambdas, but for those, it's trivial because the opening and closing braces will be right next to each other (and also we can completely ignore for namespaces).

You can write a TestTokenAnalyzer to get the UnwrappedLines for a snippet of code and write unit tests using that.

[krasimir]

Also a line may both close and open a block, as the } else { line in:

if (c()) {
  a();
} else {
  b();
}

I still need a bit of explanation. Here's my reasoning:

1. Now, FormatToken already has a member MatchingParen, so the most straightforward thing would be to store the information at the FormatToken level.

However, that would directly link tokens from different unwrapped lines, which is not a sound design, because we want to keep unwrapped lines relatively separated?

2. For this reason it might be better to store the matching info in the unwrapped line?

Is this correct?

[krasimir]

• Make the parser do the matching of begin/end line blocks

[krasimir]

• Make the namespace name matcher simpler

[krasimir]

I think this is ready for another range of reviews!

[krasimir]

And of course, the remaining question is how exactly do we integrate this into clang-format. Do we make the namespace fixer always fix namespace comments during reformat()?

[krasimir]

• Add tests for macros

[djasper]

lib/Format/NamespaceEndCommentsFixer.cpp

35

I don't think you'll ever need the actual tokens. How about just returning the name here?

59

I’d leave the entire CRLF handling out. We must not replicate this N times. For now, I’d rely on the subsequent formatting step to tidy this up and if there are reasons not to do that, we’d want to have this functionality in WhitespaceManager or such.

83

I think you can also have anonymous namespaces that require a subsequent newline, so returning earlier is probably wrong.

163

Don't test this here. Only call fixEndComment when you actually know you want to fix.

166

Similarly, don’t test isShort here, test it outside.

187

I'd write this as:

if (!AnnotatedLines[i].Affected || !AnnotatedLines[i].startsWith(tok::r_brace))
  continue;
const AnnotatedLine *EndLine = AnnotatedLines[i];
size_t StartLineIndex = EndLine->MatchingOpeningBlockLine;
if (StartLineIndex == UnwrappedLine::kInvalidIndex)
  continue;
assert(StartLineIndex < E);
const AnnotatedLine *StartLine = AnnotatedLines[StartLineIndex];
if (!StartLine->startsWith(tok::kw_namespace) &&
    !StartLine->startsWith(tok::kw_inline, tok::kw_namespace))
  continue;

...

212

So, what happens if (I+1 >= E)? Wouldn’t you then have an uninitialized value in NextAfterEndComment that you then dereference? The only thing you are using this for is to compute whether or not you need a newline. I’d compute this here. Then all of the other functions can be entirely ignorant about the next line. The design principle I’d use here is to pass a long as little information as necessary.

214

Once you have done most of the above, I think the entire structure NamespaceBlock becomes unnecessary. All you really need in fixEndComment is:

• The namespace name
• The r_brace token
• Whether or not you need to add a newline

[krasimir]

• Remove NamespaceBlock and address review comments

lib/Format/NamespaceEndCommentsFixer.cpp

166

I can't really test for isShort outside because I still want to update wrong namespace end comments for short namespaces.
The combination of your two comments has the affect to make fixComment virtually useless, I'll just inline it.

187

I'll rewrite it as you suggest except for the last if statement: I need the namespace token later for generating the name, so I'll have to skip over the namespace and inline anyways.

214

plus the namespace token to determine if the block is short. I guess I'll just inline the definition of the namespace block.

[krasimir]

• Removed redundant variable StartLine

[krasimir]

I think this is ready for another round of reviews. Sorry for the overly complicated design.

[djasper]

Looks good.

About hooking it up to clang-format: I suggest a boolean flag for now to enable/disable it.

lib/Format/NamespaceEndCommentsFixer.cpp

48

Remove

[krasimir]

Do we want this on or off by default?

[djasper]

On by default for LLVM and Google style, don't know about the others.

[krasimir]

• Remove redundant semicolon

include/clang/Format/Format.h

 /// \brief Clean up any erroneous/redundant code in the given \p Ranges in \p
 /// Code.
 ///
 /// Returns the ``Replacements`` that clean up all \p Ranges in \p Code.
 tooling::Replacements cleanup(const FormatStyle &Style, StringRef Code,
                               ArrayRef<tooling::Range> Ranges,
                               StringRef FileName = "<stdin>");
 
+/// \brief Fix namespace end comments in the given \p Ranges in \p Code.
+///
+/// Returns the ``Replacements`` that fix the namespace comments in all
+/// \p Ranges in \p Code.
+tooling::Replacements fixNamespaceEndComments(const FormatStyle &Style,
+                                              StringRef Code,
+                                              ArrayRef<tooling::Range> Ranges,
+                                              StringRef FileName = "<stdin>");
+
 /// \brief Returns the ``LangOpts`` that the formatter expects you to set.
 ///
 /// \param Style determines specific settings for lexing mode.
 LangOptions getFormattingLangOpts(const FormatStyle &Style = getLLVMStyle());
 
 /// \brief Description to be used for help text for a ``llvm::cl`` option for
 /// specifying format style. The description is closely related to the operation
 /// of ``getStyle()``.

lib/Format/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangFormat
   AffectedRangeManager.cpp
   BreakableToken.cpp
   ContinuationIndenter.cpp
   Format.cpp
   FormatToken.cpp
   FormatTokenLexer.cpp
+  NamespaceEndCommentsFixer.cpp
   SortJavaScriptImports.cpp
   TokenAnalyzer.cpp
   TokenAnnotator.cpp
   UnwrappedLineFormatter.cpp
   UnwrappedLineParser.cpp
   WhitespaceManager.cpp
 
   LINK_LIBS
   clangBasic
   clangLex
   clangToolingCore
   )

lib/Format/Format.cpp

 /// split into separate files as we go.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/Format/Format.h"
 #include "AffectedRangeManager.h"
 #include "ContinuationIndenter.h"
 #include "FormatTokenLexer.h"
+#include "NamespaceEndCommentsFixer.h"
 #include "SortJavaScriptImports.h"
 #include "TokenAnalyzer.h"
 #include "TokenAnnotator.h"
 #include "UnwrappedLineFormatter.h"
 #include "UnwrappedLineParser.h"
 #include "WhitespaceManager.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
for (AnnotatedLine *Line : Lines) {
             break;
           }
         }
       }
     }
   }
 };
 
+static bool inputUsesCRLF(StringRef Text) {
+  return Text.count('\r') * 2 > Text.count('\n');
+}
+
 class Formatter : public TokenAnalyzer {
 public:
   Formatter(const Environment &Env, const FormatStyle &Style,
             bool *IncompleteFormat)
       : TokenAnalyzer(Env, Style), IncompleteFormat(IncompleteFormat) {}
 
   tooling::Replacements
   analyze(TokenAnnotator &Annotator,
UnwrappedLineFormatter(&Indenter, &Whitespaces, Style, Tokens.getKeywords(),
         .format(AnnotatedLines);
     for (const auto &R : Whitespaces.generateReplacements())
       if (Result.add(R))
         return Result;
     return Result;
   }
 
 private:
-
-  static bool inputUsesCRLF(StringRef Text) {
-    return Text.count('\r') * 2 > Text.count('\n');
-  }
-
   bool
   hasCpp03IncompatibleFormat(const SmallVectorImpl<AnnotatedLine *> &Lines) {
     for (const AnnotatedLine *Line : Lines) {
       if (hasCpp03IncompatibleFormat(Line->Children))
         return true;
       for (FormatToken *Tok = Line->First->Next; Tok; Tok = Tok->Next) {
         if (Tok->WhitespaceRange.getBegin() == Tok->WhitespaceRange.getEnd()) {
           if (Tok->is(tok::coloncolon) && Tok->Previous->is(TT_TemplateOpener))
tooling::Replacements cleanup(const FormatStyle &Style, StringRef Code,
                               ArrayRef<tooling::Range> Ranges,
                               StringRef FileName) {
   std::unique_ptr<Environment> Env =
       Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
   Cleaner Clean(*Env, Style);
   return Clean.process();
 }
 
+tooling::Replacements fixNamespaceEndComments(const FormatStyle &Style,
+                                              StringRef Code,
+                                              ArrayRef<tooling::Range> Ranges,
+                                              StringRef FileName) {
+  std::unique_ptr<Environment> Env =
+      Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
+  NamespaceEndCommentsFixer Fix(*Env, Style, inputUsesCRLF(Code));
+  return Fix.process();
+}
+
 LangOptions getFormattingLangOpts(const FormatStyle &Style) {
   LangOptions LangOpts;
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.CPlusPlus14 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.LineComment = 1;
   bool AlternativeOperators = Style.Language == FormatStyle::LK_Cpp;
   LangOpts.CXXOperatorNames = AlternativeOperators ? 1 : 0;

lib/Format/NamespaceEndCommentsFixer.h

+//===--- NamespaceEndCommentsFixer.h ----------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief This file declares NamespaceEndCommentsFixer, a TokenAnalyzer that
+/// fixes namespace end comments.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_FORMAT_NAMESPACEENDCOMMENTSFIXER_H
+#define LLVM_CLANG_LIB_FORMAT_NAMESPACEENDCOMMENTSFIXER_H
+
+#include "TokenAnalyzer.h"
+
+namespace clang {
+namespace format {
+
+class NamespaceEndCommentsFixer : public TokenAnalyzer {
+public:
+  NamespaceEndCommentsFixer(const Environment &Env, const FormatStyle &Style,
+                            bool UsesCRLF);
+
+  tooling::Replacements
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override;
+
+private:
+  bool UsesCRLF;
+};
+
+} // end namespace format
+} // end namespace clang
+
+#endif

lib/Format/NamespaceEndCommentsFixer.cpp

+//===--- NamespaceEndCommentsFixer.cpp --------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief This file implements NamespaceEndCommentsFixer, a TokenAnalyzer that
+/// fixes namespace end comments.
+///
+//===----------------------------------------------------------------------===//
+
+#include "NamespaceEndCommentsFixer.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/Regex.h"
+
+#define DEBUG_TYPE "namespace-end-comments-fixer"
+
+namespace clang {
+namespace format {
+
+namespace {
+
+// Given a namespace token, computes the sequence of tokens that constitute that
+// namespace name. For example, for:
+//
+// namespace ::A::B { ... }
+//
+// this function returns [::, A, ::, B].
+static SmallVector<const FormatToken *, 1>
+computeNamespaceNameTokens(const FormatToken *NamespaceTok) {
+  assert(NamespaceTok && NamespaceTok->is(tok::kw_namespace) &&
+         "expecting a namespace token");
+  // Matches (modulo comments) an optional '::', followed by a keyword, followed
+  // by a repetition of: ('::' followed by a keyword):
+  // (::)? kw (:: kw)*
+  SmallVector<const FormatToken *, 1> NameToks;
+  const FormatToken *LastColonColon = nullptr;
+  const FormatToken *Tok = NamespaceTok->getNextNonComment();
+  // Match the first (optional) '::'.
+  if (Tok->is(tok::coloncolon)) {
+    LastColonColon = Tok;
+    Tok = Tok->getNextNonComment();
+  }
+  while (Tok && Tok->is(tok::identifier)) {
+    // Match a keyword followed by '::'.
+    if (LastColonColon) {
+      NameToks.push_back(LastColonColon);
+      LastColonColon = nullptr;
+    }
+    NameToks.push_back(Tok);
+    Tok = Tok->getNextNonComment();
+    if (Tok && Tok->is(tok::coloncolon)) {
+      LastColonColon = Tok;
+      Tok = Tok->getNextNonComment();
+    }
+  }
+  return NameToks;
+}
+
+// The maximal number of lines that a short namespace spans.
+// Short namespaces don't need an end comment.
+static const int kShortNamespaceMaxLines = 1;
+
+// Matches a valid namespace end comment.
+// Valid namespace end comments don't need to be edited.
+static llvm::Regex kNamespaceCommentPattern =
+    llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
+                "namespace( +([a-zA-Z0-9:_]+))?\\.? *(\\*/)?$",
+                llvm::Regex::IgnoreCase);
+
+static StringRef newline(bool UsesCRLF) { return UsesCRLF ? "\r\n" : "\n"; }
+
+struct Namespace {
+  Namespace(const FormatToken *NamespaceToken, const SourceManager &SourceMgr,
+            bool UsesCRLF)
+      : NamespaceToken(NamespaceToken),
+        NamespaceNameTokens(computeNamespaceNameTokens(NamespaceToken)),
+        SourceMgr(SourceMgr), UsesCRLF(UsesCRLF), RBrace(nullptr),
+        RBraceLine(nullptr), NextAfterEndComment(nullptr) {}
+
+  bool isAnonymous() const { return NamespaceNameTokens.empty(); }
+
+  bool isShort() const {
+    int StartLine =
+        SourceMgr.getSpellingLineNumber(NamespaceToken->Tok.getLocation());
+    int EndLine = SourceMgr.getSpellingLineNumber(RBrace->Tok.getLocation());
+    return EndLine - StartLine + 1 <= kShortNamespaceMaxLines;
+  }
+
+  std::string getNamespaceName() const {
+    std::string name = "";
+    for (const FormatToken *Tok : NamespaceNameTokens) {
+      name += Tok->TokenText;
+    }
+    return name;
+  }
+
+  bool mustAddNewline() const {
+    assert(NextAfterEndComment);
+    return NextAfterEndComment->NewlinesBefore == 0 &&
+           NextAfterEndComment->isNot(tok::eof);
+  }
+
+  std::string getNamespaceEndLineCommentText(int SpacesBefore) const {
+    std::string text(SpacesBefore, ' ');
+    text += "// namespace";
+    if (isAnonymous())
+      return text;
+    text += ' ';
+    text += getNamespaceName();
+    if (mustAddNewline()) {
+      text += newline(UsesCRLF);
+    }
+    return text;
+  }
+
+  std::string getNamespaceEndBlockCommentText(int SpacesBefore) const {
+    std::string text(SpacesBefore, ' ');
+    text += "/* namespace";
+    if (!isAnonymous()) {
+      text += ' ';
+      text += getNamespaceName();
+    }
+    text += " */";
+    if (mustAddNewline()) {
+      text += newline(UsesCRLF);
+    }
+    return text;
+  }
+
+  const FormatToken *getNextTokenAfterNamespaceName() const {
+    const FormatToken *Last = NamespaceNameTokens.empty()
+                                  ? NamespaceToken
+                                  : NamespaceNameTokens.back();
+    return Last->getNextNonComment();
+  }
+
+  bool hasEndComment() const {
+    return RBrace && RBrace->Next && RBrace->Next->is(tok::comment);
+  }
+
+  bool validEndComment() const {
+    assert(hasEndComment());
+    const FormatToken *Comment = RBrace->Next;
+    SmallVector<StringRef, 7> Groups;
+    if (kNamespaceCommentPattern.match(Comment->TokenText, &Groups)) {
+      StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
+      // Anonymous namespace comments must not mention a namespace name.
+      if (isAnonymous() && !NamespaceNameInComment.empty())
+        return false;
+      StringRef AnonymousInComment = Groups.size() > 3 ? Groups[3] : "";
+      // Named namespace comments must not mention anonymous namespace.
+      if (!isAnonymous() && !AnonymousInComment.empty())
+        return false;
+      return NamespaceNameInComment == getNamespaceName();
+    }
+    return false;
+  }
+
+  void addEndComment(tooling::Replacements *Fixes) const {
+    auto Range = CharSourceRange::getCharRange(RBrace->Tok.getEndLoc(),
+                                               RBrace->Tok.getEndLoc());
+    auto Err = Fixes->add(tooling::Replacement(
+        SourceMgr, Range, getNamespaceEndLineCommentText(/*SpacesBefore=*/1)));
+    if (Err) {
+      llvm::errs() << "Error while adding namespace end comment: "
+                   << llvm::toString(std::move(Err)) << "\n";
+    }
+  }
+
+  void updateEndComment(tooling::Replacements *Fixes) const {
+    assert(hasEndComment());
+    const FormatToken *Comment = RBrace->Next;
+    auto Range = CharSourceRange::getCharRange(
+        Comment->getStartOfNonWhitespace(), Comment->Tok.getEndLoc());
+    std::string NewComment =
+        Comment->is(TT_LineComment)
+            ? getNamespaceEndLineCommentText(/*SpacesBefore=*/0)
+            : getNamespaceEndBlockCommentText(/*SpacesBefore=*/0);
+    auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, NewComment));
+    if (Err) {
+      llvm::errs() << "Error while updating namespace end comment: "
+                   << llvm::toString(std::move(Err)) << "\n";
+    }
+  }
+
+  void fixEndComment(tooling::Replacements *Fixes) const {
+    assert(RBrace);
+    assert(RBraceLine);
+    if (!RBraceLine->Affected || RBraceLine->InPPDirective)
+      return;
+    if (hasEndComment()) {
+      if (!validEndComment()) {
+        updateEndComment(Fixes);
+      }
+    } else if (!isShort()) {
+      addEndComment(Fixes);
+    }
+  }
+
+  const FormatToken *NamespaceToken;
+  SmallVector<const FormatToken *, 1> NamespaceNameTokens;
+  const SourceManager &SourceMgr;
+  bool UsesCRLF;
+  const FormatToken *RBrace;
+  const AnnotatedLine *RBraceLine;
+  bool RBraceAffected;
+  const FormatToken *NextAfterEndComment;
+};
+
+static const FormatToken *
+getNextTokenAfterEndComment(const FormatToken *RBrace) {
+  assert(RBrace);
+  const FormatToken *Tok = RBrace->Next;
+  if (Tok && Tok->is(tok::comment))
+    Tok = Tok->Next;
+  return Tok;
+}
+
+struct LBrace {
+  const FormatToken *LBraceToken;
+  Namespace *NamespaceOpened;
+};
+
+void appendNamespaces(SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+                      const SourceManager &SourceMgr, bool UsesCRLF,
+                      SmallVectorImpl<std::unique_ptr<Namespace>> *Namespaces) {
+  assert(Namespaces);
+  SmallVector<LBrace, 4> LBraceStack;
+  Namespace *NamespaceNeedingNext = nullptr;
+  for (const AnnotatedLine *Line : AnnotatedLines) {
+    const FormatToken *Tok = Line->First;
+    assert(Tok);
+    if (NamespaceNeedingNext) {
+      assert(!NamespaceNeedingNext->NextAfterEndComment);
+      NamespaceNeedingNext->NextAfterEndComment = Tok;
+      NamespaceNeedingNext = nullptr;
+    }
+    // Detect "(inline)? namespace" in the beginning of a line.
+    if (Tok->is(tok::kw_inline))
+      Tok = Tok->getNextNonComment();
+    if (Tok && Tok->is(tok::kw_namespace)) {
+      Namespaces->push_back(
+          llvm::make_unique<Namespace>(Tok, SourceMgr, UsesCRLF));
+      Tok = Namespaces->back()->getNextTokenAfterNamespaceName();
+      if (Tok && Tok->is(tok::l_brace)) {
+        // Attach this namespace to the left brace.
+        LBraceStack.push_back({Tok, Namespaces->back().get()});
+        Tok = Tok->getNextNonComment();
+      }
+    }
+    while (Tok) {
+      if (Tok->is(tok::l_brace)) {
+        LBraceStack.push_back({Tok, /*NamespaceOpened=*/nullptr});
+      } else if (Tok->is(tok::r_brace) && !LBraceStack.empty()) {
+        Namespace *LastNamespaceOpenend = LBraceStack.back().NamespaceOpened;
+        if (LastNamespaceOpenend) {
+          LastNamespaceOpenend->RBrace = Tok;
+          LastNamespaceOpenend->RBraceLine = Line;
+          LastNamespaceOpenend->NextAfterEndComment =
+              getNextTokenAfterEndComment(Tok);
+          if (!LastNamespaceOpenend->NextAfterEndComment) {
+            NamespaceNeedingNext = LastNamespaceOpenend;
+          }
+        }
+        LBraceStack.pop_back();
+      }
+      Tok = Tok->Next;
+    }
+  }
+}
+
+} // end anonymous namespace
+
+NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
+                                                     const FormatStyle &Style,
+                                                     bool UsesCRLF)
+    : TokenAnalyzer(Env, Style), UsesCRLF(UsesCRLF) {}
+
+tooling::Replacements NamespaceEndCommentsFixer::analyze(
+    TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+    FormatTokenLexer &Tokens) {
+  AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
+                                        AnnotatedLines.end());
+  tooling::Replacements Fixes;
+  SmallVector<std::unique_ptr<Namespace>, 2> Namespaces;
+  appendNamespaces(AnnotatedLines, Env.getSourceManager(), UsesCRLF,
+                   &Namespaces);
+  for (unsigned I = 0, E = Namespaces.size(); I != E; ++I) {
+    Namespaces[I]->fixEndComment(&Fixes);
+  }
+  return Fixes;
+}
+
+} // end namespace format
+} // end namespace clang

unittests/Format/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FormatTests
   CleanupTest.cpp
+  FixNamespaceEndCommentsTest.cpp
   FormatTest.cpp
   FormatTestComments.cpp
   FormatTestJava.cpp
   FormatTestJS.cpp
   FormatTestObjC.cpp
   FormatTestProto.cpp
   FormatTestSelective.cpp
   SortImportsTestJS.cpp

unittests/Format/FixNamespaceEndCommentsTest.cpp

+//===- FixNamespaceEndCommentsTest.cpp - Formatting unit tests ------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Format/Format.h"
+
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "fix-namespace-end-comments-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class FixNamespaceEndCommentTest : public ::testing::Test {
+protected:
+  std::string fixNamespaceEndComments(llvm::StringRef Code,
+                                      std::vector<tooling::Range> Ranges,
+                                      const FormatStyle &Style = getLLVMStyle()) {
+    DEBUG(llvm::errs() << "---\n");
+    DEBUG(llvm::errs() << Code << "\n\n");
+    tooling::Replacements Replaces =
+        clang::format::fixNamespaceEndComments(Style, Code, Ranges, "<stdin>");
+    auto Result = applyAllReplacements(Code, Replaces);
+    EXPECT_TRUE(static_cast<bool>(Result));
+    DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+    return *Result;
+  }
+
+  std::string
+  fixNamespaceEndComments(llvm::StringRef Code,
+                          const FormatStyle &Style = getLLVMStyle()) {
+    return fixNamespaceEndComments(
+        Code,
+        /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+};
+
+TEST_F(FixNamespaceEndCommentTest, AddsEndComment) {
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "} // namespace",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "} // namespace\n",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "}\n"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("inline namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("inline namespace A {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("namespace ::A {\n"
+            "  int i;\n"
+            "} // namespace ::A",
+            fixNamespaceEndComments("namespace ::A {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("namespace ::A::B {\n"
+            "  int i;\n"
+            "} // namespace ::A::B",
+            fixNamespaceEndComments("namespace ::A::B {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("namespace /**/::/**/A/**/::/**/B/**/ {\n"
+            "  int i;\n"
+            "} // namespace ::A::B",
+            fixNamespaceEndComments("namespace /**/::/**/A/**/::/**/B/**/ {\n"
+                                    "  int i;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "namespace B {\n"
+            "  int i;\n"
+            "} // namespace B\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "namespace B {\n"
+                                    "  int i;\n"
+                                    "}\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "  int a;\n"
+            "} // namespace A\n"
+            "namespace B {\n"
+            "  int b;\n"
+            "} // namespace B",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int a;\n"
+                                    "}\n"
+                                    "namespace B {\n"
+                                    "  int b;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "  int a1\n"
+            "} // namespace A\n"
+            "namespace A {\n"
+            "  int a2;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int a1\n"
+                                    "} // namespace A\n"
+                                    "namespace A {\n"
+                                    "  int a2;\n"
+                                    "} // namespace A"));
+  EXPECT_EQ("namespace A {\n"
+            "  int a;\n"
+            "} // namespace A\n"
+            "// comment about b\n"
+            "int b;",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int a;\n"
+                                    "} // namespace A\n"
+                                    "// comment about b\n"
+                                    "int b;"));
+
+  EXPECT_EQ("namespace A {\n"
+            "namespace B {\n"
+            "namespace C {\n"
+            "namespace D {\n"
+            "} // namespace D\n"
+            "} // namespace C\n"
+            "} // namespace B\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "namespace B {\n"
+                                    "namespace C {\n"
+                                    "namespace D {\n"
+                                    "}\n"
+                                    "}\n"
+                                    "}\n"
+                                    "}"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, AddsNewlineIfNeeded) {
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A\n"
+            " int j;",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} int j;"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A\n"
+            " namespace B {\n"
+            "  int j;\n"
+            "} // namespace B",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} namespace B {\n"
+                                    "  int j;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\r\n"
+            "  int i;\r\n"
+            "} // namespace A\r\n"
+            " int j;",
+            fixNamespaceEndComments("namespace A {\r\n"
+                                    "  int i;\r\n"
+                                    "} int j;"));
+}
+
+
+TEST_F(FixNamespaceEndCommentTest, DoesNotAddEndCommentForShortNamespace) {
+  EXPECT_EQ("namespace {}", fixNamespaceEndComments("namespace {}"));
+  EXPECT_EQ("namespace A {}", fixNamespaceEndComments("namespace A {}"));
+  EXPECT_EQ("namespace A { int i; }",
+            fixNamespaceEndComments("namespace A { int i; }"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, DoesNotAddCommentAfterNonNamespaceRBrace) {
+  EXPECT_EQ("namespace {\n"
+            "int main() {\n"
+            "  return 0;\n"
+            "}\n"
+            "} // namespace",
+            fixNamespaceEndComments("namespace {\n"
+                                    "int main() {\n"
+                                    "  return 0;\n"
+                                    "}\n"
+                                    "}"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, DoesNotAddCommentAfterUnaffectedRBrace) {
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "}",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "}",
+                                    // The range (16, 3) spans the 'int' above.
+                                    /*Ranges=*/{1, tooling::Range(16, 3)}));
+}
+
+TEST_F(FixNamespaceEndCommentTest, DoesNotAddCommentAfterRBraceInPPDirective) {
+  EXPECT_EQ("#define SAD \\\n"
+            "namespace A { \\\n"
+            "  int i; \\\n"
+            "}",
+            fixNamespaceEndComments("#define SAD \\\n"
+                                    "namespace A { \\\n"
+                                    "  int i; \\\n"
+                                    "}"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, KeepsValidEndComment) {
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "} // end anonymous namespace",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "} // end anonymous namespace"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} /* end of namespace A */",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} /* end of namespace A */"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "}   //   namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "}   //   namespace A"));
+  EXPECT_EQ("namespace A::B {\n"
+            "  int i;\n"
+            "} // end namespace A::B",
+            fixNamespaceEndComments("namespace A::B {\n"
+                                    "  int i;\n"
+                                    "} // end namespace A::B"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, UpdatesInvalidEndLineComment) {
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "} // namespace",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "} // namespace A"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} // namespace"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} //"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} //"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} // namespace A",
+            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"));
+}
+
+TEST_F(FixNamespaceEndCommentTest, UpdatesInvalidEndBlockComment) {
+  EXPECT_EQ("namespace {\n"
+            "  int i;\n"
+            "} /* namespace */",
+            fixNamespaceEndComments("namespace {\n"
+                                    "  int i;\n"
+                                    "} /* namespace A */"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} /* namespace A */",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} /* end namespace */"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} /* namespace A */",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} /**/"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} /* namespace A */",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} /* end unnamed namespace */"));
+  EXPECT_EQ("namespace A {\n"
+            "  int i;\n"
+            "} /* namespace A */",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int i;\n"
+                                    "} /* banamespace A */"));
+  EXPECT_EQ("namespace A {} /* namespace A */",
+            fixNamespaceEndComments("namespace A {} /**/"));
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang