[clang-format] Fix SeparateDefinitionBlocks issues

[ksyx]

Summary

Fixes https://github.com/llvm/llvm-project/issues/52976.

• Make no formatting for macros
• Attach comment with definition headers
• Make no change on use of empty lines at block start/end
• Fix misrecognition of keyword namespace

[ksyx]

failed on dealing with classes.

[ksyx]

Fix failing when the first line of body is comment.

[ksyx]

apply clangfmt suggestions.

[MyDeveloperDay]

clang/lib/Format/DefinitionBlockSeparator.cpp

41–42

Style.isJavaScript()

42–43

do we see this as JsExtraKeywords.kw_fcuntion?

106

I might be having a Brain f**t, but Isn't this just OpterateIndex += Direction in which case do we need the if/else?

Sorry but I find the double negative hard to read.

111

Style.isCSharp() please

112

Do we see this as an TT_AttributeSquare, this could help reduce possible false positives?

115–119

I never know if we should be using {} here?

140–141

remove {}

148

any reason why we are not looking for TargetToken->isNot(tok::l_brace)?

158

remove {}?

[MyDeveloperDay]

}

#if 0
<--- extra unwanted newline
void
func()
{
}
<--- extra unwanted newline
#endif

void bar()
{
}

[MyDeveloperDay]

namespace {
<--- extraneous newline will be removed (just checking if thats what we want?)  shouldn't it honour MaxEmptyLines?  
   /*  foo */
   void foo()
   {
   }
<--- extraneous newline will be removed (just checking if thats what we want?)  shouldn't it honour MaxEmptyLines?
}

[MyDeveloperDay]

}

#if 0
<--- extra unwanted newline
void
func()
{
}
<--- extra unwanted newline
#else
<--- extra unwanted newline
void
bar()
{
}
<--- extra unwanted newline
#endif

void bar()
{
}

[MyDeveloperDay]

I highly recommend you running this on a large code base...I'm pulling this patch down and running it on my work code base, (because I want to use this feature).

Don't take my highlighting issues as anything other than "Go faster I like it... ;-)"

But this is an definitely and improvement, and this is a great feature (one I've been doing by hand for years, so I'm super excited.)

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

115–119

We shoudn't.

And I prefer prefix in-/decrement, instead of postfix.

[ksyx]

Apply most suggestions

[ksyx]

In D116663#3222714, @MyDeveloperDay wrote:

namespace {
<--- extraneous newline will be removed (just checking if thats what we want?)  shouldn't it honour MaxEmptyLines?  
   /*  foo */
   void foo()
   {
   }
<--- extraneous newline will be removed (just checking if thats what we want?)  shouldn't it honour MaxEmptyLines?
}

I did not come out with an implementation of this currently, but one alternative solution might be to make it leave the line spacing as original code?

clang/lib/Format/DefinitionBlockSeparator.cpp

42–43

Yes, but I have no idea about what to do with this.

[MyDeveloperDay]

clang/lib/Format/DefinitionBlockSeparator.cpp

79

you need to add unit tests for this

if I have:

void foo()
{
}
<----- you are removing the empty line here
#if 1
void bar()
{
}
#endif

but I think we still want that line between them.

Personally I would NOT attempt to remove any existing lines if I have my settings as Always there should be not empty line removal in my view.

Only the "Never" case should ever be removing lines

The same the other side

#if 1
void bar()
{
}
#endif
<----- you are removing the empty line here
void baz()
{
}

and here

#endif
<----- you are removing the empty line here
namespace 
{
}

[ksyx]

Apply suggested behaviour and fix misrecognition of token namespace

[ksyx]

apply suggestion from clangfmt.

[ksyx]

apply suggestion from clangfmt.

[MyDeveloperDay]

Do we have any ifdef tests?

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

171

ok, I'm not a fan of having lambda/functions in unit tests that get reused, this is just my experience that when this fails on line 161 you have no idea which test is failing..

Sorry from my perspective there is something wrong here..verifyFormat should be able to give you what you need. personally I'd rather have it verbatium

[MyDeveloperDay]

Its definitely a lot better (LGTM), as the original changes have been committed I might suggest we land these changes to get us much closer. (and reduce the churn and comments on this review)

Whilst I think you have fixed

https://github.com/llvm/llvm-project/issues/52976

This shows some other failure scenarios

https://github.com/llvm/llvm-project/issues/45895

I kind of think those other failures might be better handled in other reviews? what do others think?

[MyDeveloperDay]

HRESULT
Foo::func()
{
}

becomes

HRESULT

Foo::func() {}

[curdeius]

Some nits only, otherwise no other comments from my part.

In D116663#3227022, @MyDeveloperDay wrote:

Its definitely a lot better (LGTM), as the original changes have been committed I might suggest we land these changes to get us much closer. (and reduce the churn and comments on this review)

+1

This shows some other failure scenarios

https://github.com/llvm/llvm-project/issues/45895

I kind of think those other failures might be better handled in other reviews? what do others think?

+1

When handling those, please split into separate issues.

clang/lib/Format/DefinitionBlockSeparator.cpp

116

142

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

63

Unrelated. Please move to another patch.

171

+1

[ksyx]

In D116663#3227022, @MyDeveloperDay wrote:

This shows some other failure scenarios

https://github.com/llvm/llvm-project/issues/45895

If I am correct, the only one unfixed in this issue would be this improvement:

With AllowShortFunctionsOnASingleLine: Inline, we sometimes group such inline functions in class definitions together

[ksyx]

• Use prefix inc/dec
• Not run test in lambda

[ksyx]

In D116663#3227029, @MyDeveloperDay wrote:

HRESULT
Foo::func()
{
}

becomes

HRESULT

Foo::func() {}

Not reproduced with the following code and command line:

int
foo(int x, int y)
{
  int r = x*y;
  return r;
}
void
bar()
{

}

bin/clang-format test.cpp -style='{AlwaysBreakAfterReturnType: All, SeparateDefinitionBlocks: Always, BreakBeforeBraces: Allman}'

But using HRESULT return type reproduced this, with -debug option it shows it as an identifier while this is not for primitive types like void.

[ksyx]

rebase

[MyDeveloperDay]

Seem related to the number of characters used in the type?

void foo()
{}

LRES
Foo::func() {}

LRESU

Foo::func() {}

[ksyx]

In D116663#3227691, @MyDeveloperDay wrote:

Seem related to the number of characters used in the type?

void foo()
{}

LRES
Foo::func() {}

LRESU

Foo::func() {}

----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x1592cb8 Text='LRES'
 M=1 C=1 T=FunctionDeclarationName S=1 F=0 B=0 BK=0 P=80 Name=identifier L=84 PPK=2 FakeLParens= FakeRParens=0 II=0x1592cf0 Text='barbabararr'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=85 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=r_paren L=86 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
----

============================

----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x2e8dcb8 Text='LRESU'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x2e8dcf0 Text='barbabararr'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=r_paren L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
----

============================

----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=void L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x15a0618 Text='void'
 M=1 C=1 T=FunctionDeclarationName S=1 F=0 B=0 BK=0 P=80 Name=identifier L=84 PPK=2 FakeLParens= FakeRParens=0 II=0x15a9cc0 Text='barbabararr'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=85 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=r_paren L=86 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
----

What I observed from the debug output is that it has been split into two blocks for long type name in AnnotatedToken info, while the shorter one only have one block.

[MyDeveloperDay]

....
From UnwrappedLineParser.cpp

if (FollowedByNewline && (Text.size() >= 5 || FunctionLike) &&
            tokenCanStartNewLine(*FormatTok) && Text == Text.upper()) {
          addUnwrappedLine();
          return;
        }

[ksyx]

https://github.com/llvm/llvm-project/commit/680b09ba8825311dc0fad69ed166a21e3f83bcbe

Seems like this is intent for dealing with macro but somehow went into return type

[MyDeveloperDay]

Without the SeparateDefinitionBlocks: Always the issue doesn't occur, this is unclear to me why

double
Func() {}

LRES
Foo::func() {}

LRESU
Foo::func() {}

[MyDeveloperDay]

I think that perhaps

if (Style.SeparateDefinitionBlocks != FormatStyle::SDS_Leave)
  Passes.emplace_back([&](const Environment &Env) {
    return DefinitionBlockSeparator(Env, Expanded).process();
  });

Should be moved to After the Formatter pass, thoughts?

[ksyx]

In D116663#3227813, @MyDeveloperDay wrote:

I think that perhaps

if (Style.SeparateDefinitionBlocks != FormatStyle::SDS_Leave)
  Passes.emplace_back([&](const Environment &Env) {
    return DefinitionBlockSeparator(Env, Expanded).process();
  });

Should be moved to After the Formatter pass, thoughts?

It does not seem to be working. The problem could be that when I am asking about Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition(), it will just return false when it is a single annotated line like LRESULT and return true for the line foo() following, so the program adds a new line above this "true function definition line". However, those like LRES foo() { would surely be treated as a normal definition line.

[MyDeveloperDay]

How about checking the line before in "LikelyDefinition", I may not sure I totally understand your logic... but

if (PreviousLine && !PreviousLine->endsWith(tok::semi) &&
        PreviousLine->startsWith(tok::identifier))
      return false;

So something like this

auto LikelyDefinition = [this](const AnnotatedLine *Line,
                               const AnnotatedLine *PreviousLine) {
  if (PreviousLine && !PreviousLine->endsWith(tok::semi) &&
      PreviousLine->startsWith(tok::identifier))
    return false;
  if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ||
      Line->startsWithNamespace())
    return true;

  FormatToken *CurrentToken = Line->First;
  while (CurrentToken) {
    if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_enum) ||
        (Style.isJavaScript() && CurrentToken->TokenText == "function"))
      return true;
    CurrentToken = CurrentToken->Next;
  }
  return false;
};

This fixed this capitialized return type > 5 chars (which for people using clang-format to format windows code with DWORD,DWORD_PTR,DOUBLE,HRESULT,LRESULT this would be pretty bad.

[ksyx]

I see your point, but I think the problem is more likely to be coming from the token annotator that detected it as a macro and just start a new line?

https://github.com/llvm/llvm-project/commit/680b09ba8825311dc0fad69ed166a21e3f83bcbe

[ksyx]

Hi, just to confirm, are there anything needed to be improved for this patch or it is ready to be committed? Thanks

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

165

Otherwise it is really not clear what that zero should mean.

[ksyx]

• add comments for direction = 0
• use true/false instead of 0/1
• rebase

clang/lib/Format/DefinitionBlockSeparator.cpp

std::pair<tooling::Replacements, unsigned> DefinitionBlockSeparator::analyze(
   AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
   tooling::Replacements Result;
   separateBlocks(AnnotatedLines, Result);
   return {Result, 0};
 }
 
 void DefinitionBlockSeparator::separateBlocks(
     SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result) {
+  const bool IsNeverStyle =
+      Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never;
   auto LikelyDefinition = [this](const AnnotatedLine *Line) {
-    if (Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition())
+    if ((Line->MightBeFunctionDecl && Line->mightBeFunctionDefinition()) ||
+        Line->startsWithNamespace())
       return true;
     FormatToken *CurrentToken = Line->First;
     while (CurrentToken) {
-      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct,
-                                tok::kw_namespace, tok::kw_enum) ||
-          (Style.Language == FormatStyle::LK_JavaScript &&
-           CurrentToken->TokenText == "function"))
+      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct, tok::kw_enum) ||
+          (Style.isJavaScript() && CurrentToken->TokenText == "function"))
         return true;
       CurrentToken = CurrentToken->Next;
     }
     return false;
   };
   unsigned NewlineCount =
       (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always ? 1 : 0) + 1;
   WhitespaceManager Whitespaces(
       Env.getSourceManager(), Style,
       Style.DeriveLineEnding
           ? WhitespaceManager::inputUsesCRLF(
                 Env.getSourceManager().getBufferData(Env.getFileID()),
                 Style.UseCRLF)
           : Style.UseCRLF);
-  for (unsigned I = 0; I < Lines.size(); I++) {
+  for (unsigned I = 0; I < Lines.size(); ++I) {
     const auto &CurrentLine = Lines[I];
+    if (CurrentLine->InPPDirective)
+      continue;
     FormatToken *TargetToken = nullptr;
     AnnotatedLine *TargetLine;
     auto OpeningLineIndex = CurrentLine->MatchingOpeningBlockLineIndex;
+    AnnotatedLine *OpeningLine = nullptr;
     const auto InsertReplacement = [&](const int NewlineToInsert) {
       assert(TargetLine);
       assert(TargetToken);
 
       // Do not handle EOF newlines.
       if (TargetToken->is(tok::eof) && NewlineToInsert > 0)
         return;
       if (!TargetLine->Affected)
         return;
       Whitespaces.replaceWhitespace(*TargetToken, NewlineToInsert,
                                     TargetToken->SpacesRequiredBefore - 1,
                                     TargetToken->StartsColumn);
     };
+    const auto IsPPConditional = [&](const size_t LineIndex) {
+      const auto &Line = Lines[LineIndex];
+      return Line->First->is(tok::hash) && Line->First->Next &&
+             Line->First->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_else,
+                                        tok::pp_ifndef, tok::pp_elifndef,
+                                        tok::pp_elifdef, tok::pp_elif,
+                                        tok::pp_endif);
+    };
     const auto FollowingOtherOpening = [&]() {
       return OpeningLineIndex == 0 ||
-             Lines[OpeningLineIndex - 1]->Last->opensScope();
+             Lines[OpeningLineIndex - 1]->Last->opensScope() ||
+             IsPPConditional(OpeningLineIndex - 1);
     };
     const auto HasEnumOnLine = [CurrentLine]() {
       FormatToken *CurrentToken = CurrentLine->First;
       while (CurrentToken) {
         if (CurrentToken->is(tok::kw_enum))
           return true;
         CurrentToken = CurrentToken->Next;
       }
       return false;
     };
 
     bool IsDefBlock = false;
+    const auto MayPrecedeDefinition = [&](const int Direction = -1) {
+      const size_t OperateIndex = OpeningLineIndex + Direction;
+      assert(OperateIndex < Lines.size());
+      const auto &OperateLine = Lines[OperateIndex];
+      return (Style.isCSharp() && OperateLine->First->is(TT_AttributeSquare)) ||
+             OperateLine->First->is(tok::comment);
+    };
 
     if (HasEnumOnLine()) {
       // We have no scope opening/closing information for enum.
       IsDefBlock = true;
       OpeningLineIndex = I;
-      TargetLine = CurrentLine;
-      TargetToken = CurrentLine->First;
+      while (OpeningLineIndex > 0 && MayPrecedeDefinition())
+        --OpeningLineIndex;
+      OpeningLine = Lines[OpeningLineIndex];
+      TargetLine = OpeningLine;
+      TargetToken = TargetLine->First;
       if (!FollowingOtherOpening())
         InsertReplacement(NewlineCount);
-      else
+      else if (IsNeverStyle)
         InsertReplacement(OpeningLineIndex != 0);
+      TargetLine = CurrentLine;
+      TargetToken = TargetLine->First;
       while (TargetToken && !TargetToken->is(tok::r_brace))
         TargetToken = TargetToken->Next;
       if (!TargetToken) {
         while (I < Lines.size() && !Lines[I]->First->is(tok::r_brace))
           ++I;
       }
     } else if (CurrentLine->First->closesScope()) {
       if (OpeningLineIndex > Lines.size())
         continue;
       // Handling the case that opening bracket has its own line.
-      OpeningLineIndex -= Lines[OpeningLineIndex]->First->TokenText == "{";
-      AnnotatedLine *OpeningLine = Lines[OpeningLineIndex];
+      OpeningLineIndex -= Lines[OpeningLineIndex]->First->is(tok::l_brace);
+      OpeningLine = Lines[OpeningLineIndex];
       // Closing a function definition.
       if (LikelyDefinition(OpeningLine)) {
         IsDefBlock = true;
-        if (OpeningLineIndex > 0) {
-          OpeningLineIndex -=
-              Style.Language == FormatStyle::LK_CSharp &&
-              Lines[OpeningLineIndex - 1]->First->is(tok::l_square);
-          OpeningLine = Lines[OpeningLineIndex];
-        }
+        while (OpeningLineIndex > 0 && MayPrecedeDefinition())
+          --OpeningLineIndex;
         TargetLine = OpeningLine;
         TargetToken = TargetLine->First;
         if (!FollowingOtherOpening()) {
           // Avoid duplicated replacement.
-          if (!TargetToken->opensScope())
+          if (TargetToken->isNot(tok::l_brace))
             InsertReplacement(NewlineCount);
-        } else
+        } else if (IsNeverStyle)
           InsertReplacement(OpeningLineIndex != 0);
       }
     }
 
     // Not the last token.
     if (IsDefBlock && I + 1 < Lines.size()) {
-      TargetLine = Lines[I + 1];
+      OpeningLineIndex = I + 1;
+      TargetLine = Lines[OpeningLineIndex];
       TargetToken = TargetLine->First;
 
       // No empty line for continuously closing scopes. The token will be
       // handled in another case if the line following is opening a
       // definition.
-      if (!TargetToken->closesScope()) {
-        if (!LikelyDefinition(TargetLine))
+      if (!TargetToken->closesScope() && !IsPPConditional(OpeningLineIndex)) {
+        // Check whether current line may be precedings of a definition line.
+        while (OpeningLineIndex + 1 < Lines.size() &&
+               MayPrecedeDefinition(/*Direction=*/0))
+          ++OpeningLineIndex;
+        TargetLine = Lines[OpeningLineIndex];
+        if (!LikelyDefinition(TargetLine)) {
+          TargetLine = Lines[I + 1];
+          TargetToken = TargetLine->First;
           InsertReplacement(NewlineCount);
-      } else {
+        }
+      } else if (IsNeverStyle) {
+        TargetLine = Lines[I + 1];
+        TargetToken = TargetLine->First;
         InsertReplacement(OpeningLineIndex != 0);
       }
     }
   }
   for (const auto &R : Whitespaces.generateReplacements())
     // The add method returns an Error instance which simulates program exit
     // code through overloading boolean operator, thus false here indicates
     // success.
     if (Result.add(R))
       return;
 }
 } // namespace format
 } // namespace clang

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

static void verifyFormat(llvm::StringRef Code,
 
     FormatStyle InverseStyle = Style;
     if (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always)
       InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
     else
       InverseStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
     EXPECT_EQ(ExpectedCode.str(), separateDefinitionBlocks(ExpectedCode, Style))
         << "Expected code is not stable";
-    std::string InverseResult = separateDefinitionBlocks(Code, InverseStyle);
-    EXPECT_NE(Code.str(), InverseResult)
+    std::string InverseResult =
+        separateDefinitionBlocks(ExpectedCode, InverseStyle);
+    EXPECT_NE(ExpectedCode.str(), InverseResult)
         << "Inverse formatting makes no difference";
     std::string CodeToFormat =
         HasOriginalCode ? Code.str() : removeEmptyLines(Code);
     std::string Result = separateDefinitionBlocks(CodeToFormat, Style);
     EXPECT_EQ(ExpectedCode.str(), Result) << "Test failed. Formatted:\n"
                                           << Result;
   }
 
   static std::string removeEmptyLines(llvm::StringRef Code) {
verifyFormat("namespace foo {\n"
                Style);
 
   verifyFormat("enum Foo { FOO, BAR };\n"
                "\n"
                "enum Bar { FOOBAR, BARFOO };\n",
                Style);
 }
 
+TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) {
+  // Returns a std::pair of two strings, with the first one for passing into
+  // Always test and the second one be the expected result of the first string.
+  auto MakeUntouchTest = [&](std::string BlockHeader, std::string BlockChanger,
+                             std::string BlockFooter, bool BlockEndNewLine) {
+    std::string CodePart1 = "enum Foo { FOO, BAR };\n"
+                            "\n"
+                            "/*\n"
+                            "test1\n"
+                            "test2\n"
+                            "*/\n"
+                            "int foo(int i, int j) {\n"
+                            "  int r = i + j;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string CodePart2 = "/* Comment block in one line*/\n"
+                            "enum Bar { FOOBAR, BARFOO };\n"
+                            "\n"
+                            "int bar3(int j, int k) {\n"
+                            "  // A comment\n"
+                            "  int r = j % k;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string CodePart3 = "int bar2(int j, int k) {\n"
+                            "  int r = j / k;\n"
+                            "  return r;\n"
+                            "}\n";
+    std::string ConcatAll = BlockHeader + CodePart1 + BlockChanger + CodePart2 +
+                            BlockFooter + (BlockEndNewLine ? "\n" : "") +
+                            CodePart3;
+    return std::make_pair(BlockHeader + removeEmptyLines(CodePart1) +
+                              BlockChanger + removeEmptyLines(CodePart2) +
+                              BlockFooter + removeEmptyLines(CodePart3),
+                          ConcatAll);
+  };
+
+  FormatStyle AlwaysStyle = getLLVMStyle();
+  AlwaysStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+
+  FormatStyle NeverStyle = getLLVMStyle();
+  NeverStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
+
+  auto TestKit = MakeUntouchTest("#ifdef FOO\n\n", "\n#elifndef BAR\n\n",
+                                 "\n#endif\n\n", false);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit =
+      MakeUntouchTest("#ifdef FOO\n", "#elifndef BAR\n", "#endif\n", false);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit = MakeUntouchTest("namespace Ns {\n\n",
+                            "\n} // namespace Ns\n\n"
+                            "namespace {\n\n",
+                            "\n} // namespace\n", true);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+
+  TestKit = MakeUntouchTest("namespace Ns {\n",
+                            "} // namespace Ns\n\n"
+                            "namespace {\n",
+                            "} // namespace\n", true);
+  verifyFormat(TestKit.first, AlwaysStyle, TestKit.second);
+  verifyFormat(TestKit.second, NeverStyle, removeEmptyLines(TestKit.second));
+}
+
 TEST_F(DefinitionBlockSeparatorTest, Always) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
   std::string Prefix = "namespace {\n";
-  std::string Postfix = "enum Foo { FOO, BAR };\n"
-                        "\n"
-                        "int foo(int i, int j) {\n"
-                        "  int r = i + j;\n"
-                        "  return r;\n"
-                        "}\n"
-                        "\n"
-                        "int i, j, k;\n"
-                        "\n"
-                        "int bar(int j, int k) {\n"
-                        "  int r = j * k;\n"
-                        "  return r;\n"
-                        "}\n"
-                        "\n"
-                        "enum Bar { FOOBAR, BARFOO };\n"
-                        "} // namespace";
-  verifyFormat(Prefix + "\n\n\n" + removeEmptyLines(Postfix), Style,
-               Prefix + Postfix);
+  std::string Infix = "\n"
+                      "// Enum test1\n"
+                      "// Enum test2\n"
+                      "enum Foo { FOO, BAR };\n"
+                      "/*\n"
+                      "test1\n"
+                      "test2\n"
+                      "*/\n"
+                      "// Foobar\n"
+                      "// Comment for function\n"
+                      "// Comment line 2\n"
+                      "// Comment line 3\n"
+                      "int bar2(int j, int k) {\n"
+                      "  int r = j / k;\n"
+                      "  return r;\n"
+                      "}\n"
+                      "\n"
+                      "/* Comment block in one line*/\n"
+                      "\n"
+                      "int bar3(int j, int k) {\n"
+                      "  // A comment\n"
+                      "  int r = j % k;\n"
+                      "  return r;\n"
+                      "}\n";
+  std::string Postfix = "\n"
+                        "} // namespace\n"
+                        "\n"
+                        "namespace T {\n"
+                        "int i, j, k;\n"
+                        "} // namespace T";
+  verifyFormat(Prefix + removeEmptyLines(Infix) + removeEmptyLines(Postfix),
+               Style, Prefix + Infix + Postfix);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, Never) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Never;
   std::string Prefix = "namespace {\n";
-  std::string Postfix = "enum Foo { FOO, BAR };\n"
+  std::string Postfix = "// Enum test1\n"
+                        "// Enum test2\n"
+                        "enum Foo { FOO, BAR };\n"
                         "\n"
+                        "/*\n"
+                        "test1\n"
+                        "test2\n"
+                        "*/\n"
                         "int foo(int i, int j) {\n"
                         "  int r = i + j;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "// Foobar\n"
                         "int i, j, k;\n"
                         "\n"
+                        "// Comment for function\n"
+                        "// Comment line 2\n"
+                        "// Comment line 3\n"
                         "int bar(int j, int k) {\n"
                         "  int r = j * k;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "int bar2(int j, int k) {\n"
+                        "  int r = j / k;\n"
+                        "  return r;\n"
+                        "}\n"
+                        "\n"
+                        "/* Comment block in one line*/\n"
                         "enum Bar { FOOBAR, BARFOO };\n"
+                        "\n"
+                        "int bar3(int j, int k) {\n"
+                        "  // A comment\n"
+                        "  int r = j % k;\n"
+                        "  return r;\n"
+                        "}\n"
                         "} // namespace";
   verifyFormat(Prefix + "\n\n\n" + Postfix, Style,
                Prefix + removeEmptyLines(Postfix));
 }
 
 TEST_F(DefinitionBlockSeparatorTest, OpeningBracketOwnsLine) {
   FormatStyle Style = getLLVMStyle();
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
-  verifyFormat("enum Foo\n"
+  verifyFormat("namespace NS\n"
+               "{\n"
+               "// Enum test1\n"
+               "// Enum test2\n"
+               "enum Foo\n"
                "{\n"
                "  FOO,\n"
                "  BAR\n"
                "};\n"
                "\n"
+               "/*\n"
+               "test1\n"
+               "test2\n"
+               "*/\n"
                "int foo(int i, int j)\n"
                "{\n"
                "  int r = i + j;\n"
                "  return r;\n"
                "}\n"
                "\n"
+               "// Foobar\n"
                "int i, j, k;\n"
                "\n"
+               "// Comment for function\n"
+               "// Comment line 2\n"
+               "// Comment line 3\n"
                "int bar(int j, int k)\n"
                "{\n"
                "  int r = j * k;\n"
                "  return r;\n"
                "}\n"
                "\n"
+               "int bar2(int j, int k)\n"
+               "{\n"
+               "  int r = j / k;\n"
+               "  return r;\n"
+               "}\n"
+               "\n"
                "enum Bar\n"
                "{\n"
                "  FOOBAR,\n"
                "  BARFOO\n"
-               "};",
+               "};\n"
+               "\n"
+               "int bar3(int j, int k)\n"
+               "{\n"
+               "  // A comment\n"
+               "  int r = j % k;\n"
+               "  return r;\n"
+               "}\n"
+               "} // namespace NS",
                Style);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, Leave) {
   FormatStyle Style = getLLVMStyle();
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;
   Style.MaxEmptyLinesToKeep = 3;
   std::string LeaveAs = "namespace {\n"
                         "\n"
+                        "// Enum test1\n"
+                        "// Enum test2\n"
                         "enum Foo { FOO, BAR };\n"
                         "\n\n\n"
+                        "/*\n"
+                        "test1\n"
+                        "test2\n"
+                        "*/\n"
                         "int foo(int i, int j) {\n"
                         "  int r = i + j;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "// Foobar\n"
                         "int i, j, k;\n"
                         "\n"
+                        "// Comment for function\n"
+                        "// Comment line 2\n"
+                        "// Comment line 3\n"
                         "int bar(int j, int k) {\n"
                         "  int r = j * k;\n"
                         "  return r;\n"
                         "}\n"
                         "\n"
+                        "int bar2(int j, int k) {\n"
+                        "  int r = j / k;\n"
+                        "  return r;\n"
+                        "}\n"
+                        "\n"
+                        "// Comment for inline enum\n"
                         "enum Bar { FOOBAR, BARFOO };\n"
+                        "int bar3(int j, int k) {\n"
+                        "  // A comment\n"
+                        "  int r = j % k;\n"
+                        "  return r;\n"
+                        "}\n"
                         "} // namespace";
   verifyFormat(LeaveAs, Style, LeaveAs);
 }
 
 TEST_F(DefinitionBlockSeparatorTest, CSharp) {
   FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
   Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
   Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
   Style.AllowShortEnumsOnASingleLine = false;
   verifyFormat("namespace {\r\n"
                "public class SomeTinyClass {\r\n"
                "  int X;\r\n"
                "}\r\n"
                "\r\n"
                "public class AnotherTinyClass {\r\n"
                "  int Y;\r\n"
                "}\r\n"
                "\r\n"
                "internal static String toString() {\r\n"
                "}\r\n"
                "\r\n"
+               "// Comment for enum\r\n"
                "public enum var {\r\n"
                "  none,\r\n"
                "  @string,\r\n"
                "  bool,\r\n"
                "  @enum\r\n"
                "}\r\n"
                "\r\n"
+               "// Test\r\n"
                "[STAThread]\r\n"
                "static void Main(string[] args) {\r\n"
                "  Console.WriteLine(\"HelloWorld\");\r\n"
                "}\r\n"
                "\r\n"
                "static decimal Test() {\r\n"
                "}\r\n"
                "}\r\n"