[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

37 ↗

(On Diff #89413)

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?

109 ↗

(On Diff #89413)

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

110 ↗

(On Diff #89413)

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

115 ↗

(On Diff #89413)

No braces for single-statement ifs.

183 ↗

(On Diff #89413)

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

196 ↗

(On Diff #89413)

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

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

229 ↗

(On Diff #89413)

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

231 ↗

(On Diff #89413)

Why unique_ptr<Namespace> instead of just Namespace?

[klimek]

lib/Format/NamespaceEndCommentsFixer.cpp

110 ↗

(On Diff #89413)

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

58 ↗

(On Diff #89853)

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.

82 ↗

(On Diff #89853)

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

165 ↗

(On Diff #89853)

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

211 ↗

(On Diff #89853)

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.

213 ↗

(On Diff #89853)

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

34 ↗

(On Diff #89668)

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

162 ↗

(On Diff #89668)

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

186 ↗

(On Diff #89668)

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;

...

[krasimir]

• Remove NamespaceBlock and address review comments

lib/Format/NamespaceEndCommentsFixer.cpp

165 ↗

(On Diff #89853)

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.

213 ↗

(On Diff #89853)

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

186 ↗

(On Diff #89668)

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.

[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

47 ↗

(On Diff #89867)

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

cfe/trunk/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()``.

cfe/trunk/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
   )

cfe/trunk/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"
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);
+  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.IsCpp();
   LangOpts.CXXOperatorNames = AlternativeOperators ? 1 : 0;

cfe/trunk/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);
+
+  tooling::Replacements
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override;
+};
+
+} // end namespace format
+} // end namespace clang
+
+#endif

cfe/trunk/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 {
+// 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);
+
+// Computes the name of a namespace given the namespace token.
+// Returns "" for anonymous namespace.
+std::string computeName(const FormatToken *NamespaceTok) {
+  assert(NamespaceTok && NamespaceTok->is(tok::kw_namespace) &&
+         "expecting a namespace token");
+  std::string name = "";
+  // Collects all the non-comment tokens between 'namespace' and '{'.
+  const FormatToken *Tok = NamespaceTok->getNextNonComment();
+  while (Tok && !Tok->is(tok::l_brace)) {
+    name += Tok->TokenText;
+    Tok = Tok->getNextNonComment();
+  }
+  return name;
+}
+
+std::string computeEndCommentText(StringRef NamespaceName, bool AddNewline) {
+  std::string text = "// namespace";
+  if (!NamespaceName.empty()) {
+    text += ' ';
+    text += NamespaceName;
+  }
+  if (AddNewline)
+    text += '\n';
+  return text;
+}
+
+bool isShort(const FormatToken *NamespaceTok, const FormatToken *RBraceTok,
+             const SourceManager &SourceMgr) {
+  int StartLine =
+      SourceMgr.getSpellingLineNumber(NamespaceTok->Tok.getLocation());
+  int EndLine = SourceMgr.getSpellingLineNumber(RBraceTok->Tok.getLocation());
+  return EndLine - StartLine + 1 <= kShortNamespaceMaxLines;
+}
+
+bool hasEndComment(const FormatToken *RBraceTok) {
+  return RBraceTok->Next && RBraceTok->Next->is(tok::comment);
+}
+
+bool validEndComment(const FormatToken *RBraceTok, StringRef NamespaceName) {
+  assert(hasEndComment(RBraceTok));
+  const FormatToken *Comment = RBraceTok->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 (NamespaceName.empty() && !NamespaceNameInComment.empty())
+      return false;
+    StringRef AnonymousInComment = Groups.size() > 3 ? Groups[3] : "";
+    // Named namespace comments must not mention anonymous namespace.
+    if (!NamespaceName.empty() && !AnonymousInComment.empty())
+      return false;
+    return NamespaceNameInComment == NamespaceName;
+  }
+  return false;
+}
+
+void addEndComment(const FormatToken *RBraceTok, StringRef EndCommentText,
+                   const SourceManager &SourceMgr,
+                   tooling::Replacements *Fixes) {
+  auto EndLoc = RBraceTok->Tok.getEndLoc();
+  auto Range = CharSourceRange::getCharRange(EndLoc, EndLoc);
+  auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, EndCommentText));
+  if (Err) {
+    llvm::errs() << "Error while adding namespace end comment: "
+                 << llvm::toString(std::move(Err)) << "\n";
+  }
+}
+
+void updateEndComment(const FormatToken *RBraceTok, StringRef EndCommentText,
+                      const SourceManager &SourceMgr,
+                      tooling::Replacements *Fixes) {
+  assert(hasEndComment(RBraceTok));
+  const FormatToken *Comment = RBraceTok->Next;
+  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";
+  }
+}
+} // 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;
+  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->isNot(tok::kw_namespace))
+      continue;
+    const FormatToken *RBraceTok = EndLine->First;
+    const std::string NamespaceName = computeName(NamespaceTok);
+    bool AddNewline = (I + 1 < E) &&
+                      AnnotatedLines[I + 1]->First->NewlinesBefore == 0 &&
+                      AnnotatedLines[I + 1]->First->isNot(tok::eof);
+    const std::string EndCommentText =
+        computeEndCommentText(NamespaceName, AddNewline);
+    if (!hasEndComment(RBraceTok)) {
+      if (!isShort(NamespaceTok, RBraceTok, SourceMgr))
+        addEndComment(RBraceTok, EndCommentText, SourceMgr, &Fixes);
+      continue;
+    }
+    if (!validEndComment(RBraceTok, NamespaceName))
+      updateEndComment(RBraceTok, EndCommentText, SourceMgr, &Fixes);
+  }
+  return Fixes;
+}
+
+} // namespace format
+} // namespace clang

cfe/trunk/lib/Format/TokenAnnotator.h

enum LineType {
   LT_PreprocessorDirective,
   LT_VirtualFunctionDecl
 };
 
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine &Line)
       : First(Line.Tokens.front().Tok), Level(Line.Level),
+        MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
         InPPDirective(Line.InPPDirective),
         MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
         IsMultiVariableDeclStmt(false), Affected(false),
         LeadingEmptyLinesAffected(false), ChildrenAffected(false) {
     assert(!Line.Tokens.empty());
 
     // Calculate Next and Previous for all tokens. Note that we must overwrite
     // Next and Previous for every token, as previous formatting runs might have
public:
 
   FormatToken *First;
   FormatToken *Last;
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
+  size_t MatchingOpeningBlockLineIndex;
   bool InPPDirective;
   bool MustBeDeclaration;
   bool MightBeFunctionDecl;
   bool IsMultiVariableDeclStmt;
 
   /// \c True if this line should be formatted, i.e. intersects directly or
   /// indirectly with one of the input ranges.
   bool Affected;

cfe/trunk/lib/Format/UnwrappedLineParser.h

struct UnwrappedLine {
 
   /// \brief The indent level of the \c UnwrappedLine.
   unsigned Level;
 
   /// \brief Whether this \c UnwrappedLine is part of a preprocessor directive.
   bool InPPDirective;
 
   bool MustBeDeclaration;
+
+  /// \brief If this \c UnwrappedLine closes a block in a sequence of lines,
+  /// \c MatchingOpeningBlockLineIndex stores the index of the corresponding
+  /// opening line. Otherwise, \c MatchingOpeningBlockLineIndex must be
+  /// \c kInvalidIndex.
+  size_t MatchingOpeningBlockLineIndex;
+
+  static const size_t kInvalidIndex = -1;
 };
 
 class UnwrappedLineConsumer {
 public:
   virtual ~UnwrappedLineConsumer() {}
   virtual void consumeUnwrappedLine(const UnwrappedLine &Line) = 0;
   virtual void finishRun() = 0;
 };

 struct UnwrappedLineNode {
   UnwrappedLineNode() : Tok(nullptr) {}
   UnwrappedLineNode(FormatToken *Tok) : Tok(Tok) {}
 
   FormatToken *Tok;
   SmallVector<UnwrappedLine, 0> Children;
 };
 
-inline UnwrappedLine::UnwrappedLine()
-    : Level(0), InPPDirective(false), MustBeDeclaration(false) {}
+inline UnwrappedLine::UnwrappedLine() : Level(0), InPPDirective(false),
+  MustBeDeclaration(false), MatchingOpeningBlockLineIndex(kInvalidIndex) {}
 
 } // end namespace format
 } // end namespace clang
 
 #endif

cfe/trunk/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
 
   unsigned InitialLevel = Line->Level;
   nextToken();
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   addUnwrappedLine();
+  size_t OpeningLineIndex =
+      Lines.empty() ? (UnwrappedLine::kInvalidIndex) : (Lines.size() - 1);
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
   if (AddLevel)
     ++Line->Level;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   nextToken(); // Munch the closing brace.
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (MunchSemi && FormatTok->Tok.is(tok::semi))
     nextToken();
   Line->Level = InitialLevel;
+  Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
 }
 
 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/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FormatTests
   CleanupTest.cpp
   FormatTest.cpp
   FormatTestComments.cpp
-  FormatTestJava.cpp
   FormatTestJS.cpp
+  FormatTestJava.cpp
   FormatTestObjC.cpp
   FormatTestProto.cpp
   FormatTestSelective.cpp
+  NamespaceEndCommentsFixerTest.cpp
   SortImportsTestJS.cpp
   SortIncludesTest.cpp
   )
 
 target_link_libraries(FormatTests
   clangBasic
   clangFormat
   clangFrontend
   clangRewrite
   clangToolingCore
   )

cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp

+//===- NamespaceEndCommentsFixerTest.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 "namespace-end-comments-fixer-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class NamespaceEndCommentsFixerTest : 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(NamespaceEndCommentsFixerTest, 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"
+                                    "}\n"
+                                    "namespace A {\n"
+                                    "  int a2;\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "  int a;\n"
+            "}// namespace A\n"
+            "// comment about b\n"
+            "int b;",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "  int a;\n"
+                                    "}\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(NamespaceEndCommentsFixerTest, 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 {\n"
+            "  int i;\n"
+            "}// namespace\n"
+            " int j;",
+            fixNamespaceEndComments("namespace {\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"
+                                    "}"));
+}
+
+TEST_F(NamespaceEndCommentsFixerTest, 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(NamespaceEndCommentsFixerTest, 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(NamespaceEndCommentsFixerTest, DoesNotAddCommentAfterRBraceInPPDirective) {
+  EXPECT_EQ("#define SAD \\\n"
+            "namespace A { \\\n"
+            "  int i; \\\n"
+            "}",
+            fixNamespaceEndComments("#define SAD \\\n"
+                                    "namespace A { \\\n"
+                                    "  int i; \\\n"
+                                    "}"));
+}
+
+TEST_F(NamespaceEndCommentsFixerTest, 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(NamespaceEndCommentsFixerTest, 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(NamespaceEndCommentsFixerTest, 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 {} /**/"));
+}
+
+TEST_F(NamespaceEndCommentsFixerTest,
+       DoesNotAddEndCommentForNamespacesControlledByMacros) {
+  EXPECT_EQ("#ifdef 1\n"
+            "namespace A {\n"
+            "#elseif\n"
+            "namespace B {\n"
+            "#endif\n"
+            "  int i;\n"
+            "}\n"
+            "}\n",
+            fixNamespaceEndComments("#ifdef 1\n"
+                                    "namespace A {\n"
+                                    "#elseif\n"
+                                    "namespace B {\n"
+                                    "#endif\n"
+                                    "  int i;\n"
+                                    "}\n"
+                                    "}\n"));
+}
+
+TEST_F(NamespaceEndCommentsFixerTest,
+       DoesNotAddEndCommentForNamespacesInMacroDeclarations) {
+  EXPECT_EQ("#ifdef 1\n"
+            "namespace A {\n"
+            "#elseif\n"
+            "namespace B {\n"
+            "#endif\n"
+            "  int i;\n"
+            "}\n"
+            "}\n",
+            fixNamespaceEndComments("#ifdef 1\n"
+                                    "namespace A {\n"
+                                    "#elseif\n"
+                                    "namespace B {\n"
+                                    "#endif\n"
+                                    "  int i;\n"
+                                    "}\n"
+                                    "}\n"));
+}
+} // end namespace
+} // end namespace format
+} // end namespace clang