[clang-format] Don't remove empty lines before namespace endings

[krasimir]

Summary

This implements an alternative to r327861, namely preserving empty lines before namespace endings.

[djasper]

Looks good.

lib/Format/NamespaceEndCommentsFixer.h

24 ↗

(On Diff #141352)

I don't understand the "if that is to be formatted part". What do you mean?

[krasimir]

lib/Format/NamespaceEndCommentsFixer.h

24 ↗

(On Diff #141352)

I'm documenting the current behavior of the implementation, getting into more detail in the next paragraph of the comments: this returns nullptr if Line is not Affected, even if otherwise would have a namespace token.

cfe/trunk/lib/Format/NamespaceEndCommentsFixer.h

 #ifndef LLVM_CLANG_LIB_FORMAT_NAMESPACEENDCOMMENTSFIXER_H
 #define LLVM_CLANG_LIB_FORMAT_NAMESPACEENDCOMMENTSFIXER_H
 
 #include "TokenAnalyzer.h"
 
 namespace clang {
 namespace format {
 
+// Finds the namespace token corresponding to a closing namespace `}`, if that
+// is to be formatted.
+// If \p Line contains the closing `}` of a namespace, is affected and is not in
+// a preprocessor directive, the result will be the matching namespace token.
+// Otherwise returns null.
+// \p AnnotatedLines is the sequence of lines from which \p Line is a member of.
+const FormatToken *
+getNamespaceToken(const AnnotatedLine *Line,
+                  const SmallVectorImpl<AnnotatedLine *> &AnnotatedLines);
+
 class NamespaceEndCommentsFixer : public TokenAnalyzer {
 public:
   NamespaceEndCommentsFixer(const Environment &Env, const FormatStyle &Style);
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override;
 };
 
 } // end namespace format
 } // end namespace clang
 
 #endif

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";
   }
 }
+} // namespace
 
 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;
   if (NamespaceTok->is(tok::l_brace)) {
     // "namespace" keyword can be on the line preceding '{', e.g. in styles
     // where BraceWrapping.AfterNamespace is true.
     if (StartLineIndex > 0)
       NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
   }
   // Detect "(inline)? namespace" in the beginning of a line.
   if (NamespaceTok->is(tok::kw_inline))
     NamespaceTok = NamespaceTok->getNextNonComment();
   if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace))
     return nullptr;
   return NamespaceTok;
 }
-} // namespace
 
 NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env,
                                                      const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 std::pair<tooling::Replacements, unsigned> NamespaceEndCommentsFixer::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {

cfe/trunk/lib/Format/UnwrappedLineFormatter.h

unsigned format(const SmallVectorImpl<AnnotatedLine *> &Lines,
                   unsigned FirstStartColumn = 0,
                   unsigned NextStartColumn = 0,
                   unsigned LastStartColumn = 0);
 
 private:
   /// \brief Add a new line and the required indent before the first Token
   /// of the \c UnwrappedLine if there was no structural parsing error.
   void formatFirstToken(const AnnotatedLine &Line,
-                        const AnnotatedLine *PreviousLine, unsigned Indent,
-                        unsigned NewlineIndent);
+                        const AnnotatedLine *PreviousLine,
+                        const SmallVectorImpl<AnnotatedLine *> &Lines,
+                        unsigned Indent, unsigned NewlineIndent);
 
   /// \brief Returns the column limit for a line, taking into account whether we
   /// need an escaped newline due to a continued preprocessor directive.
   unsigned getColumnLimit(bool InPPDirective,
                           const AnnotatedLine *NextLine) const;
 
   // Cache to store the penalty of formatting a vector of AnnotatedLines
   // starting from a specific additional offset. Improves performance if there

cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp

 //===--- UnwrappedLineFormatter.cpp - Format C++ code ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "NamespaceEndCommentsFixer.h"
 #include "UnwrappedLineFormatter.h"
 #include "WhitespaceManager.h"
 #include "llvm/Support/Debug.h"
 #include <queue>
 
 #define DEBUG_TYPE "format-formatter"
 
 namespace clang {
if (ShouldFormat && TheLine.Type == LT_Invalid && Status) {
       Status->FormatComplete = false;
       Status->Line =
           SourceMgr.getSpellingLineNumber(TheLine.First->Tok.getLocation());
     }
 
     if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (!DryRun) {
         bool LastLine = Line->First->is(tok::eof);
-        formatFirstToken(TheLine, PreviousLine,
-                         Indent,
+        formatFirstToken(TheLine, PreviousLine, Lines, Indent,
                          LastLine ? LastStartColumn : NextStartColumn + Indent);
       }
 
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       unsigned ColumnLimit = getColumnLimit(TheLine.InPPDirective, NextLine);
       bool FitsIntoOneLine =
           TheLine.Last->TotalLength + Indent <= ColumnLimit ||
           (TheLine.Type == LT_ImportStatement &&
if (ShouldFormat && TheLine.Type != LT_Invalid) {
       if (StartsNewLine)
         IndentTracker.adjustToUnmodifiedLine(TheLine);
       if (!DryRun) {
         bool ReformatLeadingWhitespace =
             StartsNewLine && ((PreviousLine && PreviousLine->Affected) ||
                               TheLine.LeadingEmptyLinesAffected);
         // Format the first token.
         if (ReformatLeadingWhitespace)
-          formatFirstToken(TheLine, PreviousLine,
+          formatFirstToken(TheLine, PreviousLine, Lines,
                            TheLine.First->OriginalColumn,
                            TheLine.First->OriginalColumn);
         else
           Whitespaces->addUntouchableToken(*TheLine.First,
                                            TheLine.InPPDirective);
 
         // Notify the WhitespaceManager about the unchanged whitespace.
         for (FormatToken *Tok = TheLine.First->Next; Tok; Tok = Tok->Next)
           Whitespaces->addUntouchableToken(*Tok, TheLine.InPPDirective);
       }
       NextLine = Joiner.getNextMergedLine(DryRun, IndentTracker);
       RangeMinLevel = UINT_MAX;
     }
     if (!DryRun)
       markFinalized(TheLine.First);
     PreviousLine = &TheLine;
   }
   PenaltyCache[CacheKey] = Penalty;
   return Penalty;
 }
 
-void UnwrappedLineFormatter::formatFirstToken(const AnnotatedLine &Line,
-                                              const AnnotatedLine *PreviousLine,
-                                              unsigned Indent,
-                                              unsigned NewlineIndent) {
+void UnwrappedLineFormatter::formatFirstToken(
+    const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
+    const SmallVectorImpl<AnnotatedLine *> &Lines, unsigned Indent,
   FormatToken &RootToken = *Line.First;
   if (RootToken.is(tok::eof)) {
     unsigned Newlines = std::min(RootToken.NewlinesBefore, 1u);
     unsigned TokenIndent = Newlines ? NewlineIndent : 0;
     Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
                                    TokenIndent);
     return;
   }
   unsigned Newlines =
       std::min(RootToken.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1);
   // Remove empty lines before "}" where applicable.
   if (RootToken.is(tok::r_brace) &&
       (!RootToken.Next ||
-       (RootToken.Next->is(tok::semi) && !RootToken.Next->Next)))
+       (RootToken.Next->is(tok::semi) && !RootToken.Next->Next)) &&
+      // Do not remove empty lines before namespace closing "}".
+      !getNamespaceToken(&Line, Lines))
     Newlines = std::min(Newlines, 1u);
   // Remove empty lines at the start of nested blocks (lambdas/arrow functions)
   if (PreviousLine == nullptr && Line.Level > 0)
     Newlines = std::min(Newlines, 1u);
   if (Newlines == 0 && !RootToken.IsFirst)
     Newlines = 1;
   if (RootToken.IsFirst && !RootToken.HasUnescapedNewline)
     Newlines = 0;

cfe/trunk/unittests/Format/FormatTest.cpp

EXPECT_EQ("void f() {\n"
                    "\n"
                    "  } else if (b) {\n"
                    "    f();\n"
                    "\n"
                    "  }\n"
                    "\n"
                    "}"));
 
-  // FIXME: This is slightly inconsistent.
+  // Don't remove empty lines before namespace endings.
   FormatStyle LLVMWithNoNamespaceFix = getLLVMStyle();
   LLVMWithNoNamespaceFix.FixNamespaceComments = false;
   EXPECT_EQ("namespace {\n"
             "int i;\n"
+            "\n"
             "}",
             format("namespace {\n"
                    "int i;\n"
                    "\n"
                    "}", LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
             "}",
             format("namespace {\n"
                    "int i;\n"
+                   "}", LLVMWithNoNamespaceFix));
+  EXPECT_EQ("namespace {\n"
+            "int i;\n"
+            "\n"
+            "};",
+            format("namespace {\n"
+                   "int i;\n"
+                   "\n"
+                   "};", LLVMWithNoNamespaceFix));
+  EXPECT_EQ("namespace {\n"
+            "int i;\n"
+            "};",
+            format("namespace {\n"
+                   "int i;\n"
+                   "};", LLVMWithNoNamespaceFix));
+  EXPECT_EQ("namespace {\n"
+            "int i;\n"
+            "\n"
+            "}",
+            format("namespace {\n"
+                   "int i;\n"
                    "\n"
                    "}"));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
             "\n"
             "} // namespace",
             format("namespace {\n"
                    "int i;\n"