[clang-format] Fix SeparateDefinitionBlocks issues

[ksyx]

Summary

• Fixes https://github.com/llvm/llvm-project/issues/53227 that wrongly indents multiline comments
• Fixes wrong detection of single-line opening bracket when used along with those only opening scopes, causing crashes due to duplicated replacements on the same token:

	void foo()
	{
	  {
		int x;
	  }
	}

• Fixes wrong recognition of first line of definition when the line starts with block comment, causing crashes due to duplicated replacements on the same token for this leads toward skipping the line starting with inline block comment:

        /*
           Some descriptions about function
        */
	/*inline*/ void bar() {
	}

• Fixes wrong recognition of enum when used as a type name rather than starting definition block, causing crashes due to duplicated replacements on the same token since both actions for enum and for definition blocks were taken place:

	void foobar(const enum EnumType e) {
	}

• Change to use function keyword for JavaScript instead of comparing strings
• Resolves formatting conflict with options EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier (prompts with --dry-run (-n) or --output-replacement-xml but no observable change)
• Recognize long (len>=5) uppercased name taking a single line as return type and fix the problem of adding newline below it, with adding new token type FunctionLikeOrFreestandingMacro and marking tokens in UnwrappedLineParser:

	void
	afunc(int x) {
	  return;
	}
	TYPENAME
	func(int x, int y) {
	  // ...
	}

• Remove redundant and repeated initialization
• Do no change to newlines before EOF

[ksyx]

Apply clangfmt's suggestion.

[MyDeveloperDay]

clang/lib/Format/DefinitionBlockSeparator.cpp

45–48

I still wonder if this can't be CurrentToken->is(JSKeywords.kw_function)

[curdeius]

Could you explain more in details the before/after of the fixed bugs, please?
The best would be to develop the patch description.

clang/lib/Format/DefinitionBlockSeparator.cpp

38

Nit and personal preference.

182

I find "bracket" ambiguous.

[ksyx]

• Use function keyword for JavaScript instead of comparing strings
• Apply review suggestions

[MyDeveloperDay]

LGTM, I'll pull it down and try it here..

[MyDeveloperDay]

I'm getting some sort of seemingly empty replacements going on, after public: I can't quite put my finger on what's happening.

[ksyx]

In D117520#3251376, @MyDeveloperDay wrote:

I'm getting some sort of seemingly empty replacements going on, after public: I can't quite put my finger on what's happening.

Not so sure what empty means here. Is it removing empty line following public: or adding, or something else?

[MyDeveloperDay]

For

class Test
{
public:
    static void foo()
    {
    }
};

$ clang-format -n test.cxx
test.cxx:3:8: warning: code should be clang-formatted [-Wclang-format-violations
]
public:
       ^

$ clang-format --output-replacements-xml test.cxx
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='20' length='5'>&#10;    </replacement>
</replacements>

but when I run it nothing changes

$ clang-format test.cxx
class Test
{
public:
    static void foo()
    {
    }
};

And no character change

[ksyx]

In D117520#3251425, @MyDeveloperDay wrote:

For

class Test
{
public:
    static void foo()
    {
    }
};

$ clang-format -n test.cxx
test.cxx:3:8: warning: code should be clang-formatted [-Wclang-format-violations
]
public:
       ^

$ clang-format --output-replacements-xml test.cxx
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='20' length='5'>&#10;    </replacement>
</replacements>

but when I run it nothing changes

$ clang-format test.cxx
class Test
{
public:
    static void foo()
    {
    }
};

And no character change

Looks like it is just replacing indent whitespaces to tabs

Sorry it does not look like that

-bash-4.4$ xxd testt.cpp
00000000: 636c 6173 7320 5465 7374 207b 0a70 7562  class Test {.pub
00000010: 6c69 633a 0a20 2073 7461 7469 6320 766f  lic:.  static vo
00000020: 6964 2066 6f6f 2829 207b 0a20 2020 2069  id foo() {.    i
00000030: 6e74 2074 3b0a 2020 2020 7265 7475 726e  nt t;.    return
00000040: 2031 3b0a 2020 7d0a 7d3b                  1;.  }.};
-bash-4.4$ cat testt.cpp | sha256sum
c8628b0d002842cdcfa532cfc4be1225a926b70ef1fc47d6ac3f9d6c46a78ad6  -
-bash-4.4$ bin/clang-format testt.cpp -style='{SeparateDefinitionBlocks: Always}' | sha256sum
c8628b0d002842cdcfa532cfc4be1225a926b70ef1fc47d6ac3f9d6c46a78ad6  -

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

79–83

Is that needed?

87–88

What if NewlineToInsert is 0? Does that explain @MyDeveloperDay s observation?

[ksyx]

clang/lib/Format/DefinitionBlockSeparator.cpp

87–88

Then the line break would be removed (concatenate with last line), which is not supposed to happen with Always style but possible at first line of Never style.

I have located the problem and roughly the solution but it takes some time to find a neat fix :)

Thanks for the feedback

[ksyx]

• Resolves formatting conflict with options EmptyLineAfterAccessModifier and EmptyLineBeforeAccessModifier (prompts with --dry-run (-n) or --output-replacement-xml but no observable change)
• Remove redundant and repeated initialization
• Do no change to newlines before EOF

[MyDeveloperDay]

ok, this is better but not quite right still.. I though I mentioned this before (please add as a unit test)

HRESULT
Foo::bar() {}

HRESULT
Foo::bar() {}

formats as

HRESULT

Foo::bar() {}

HRESULT

Foo::bar() {}

If you are windows developer using clang-format (HRESULT,LRESULT,LPSTR,LPCSTR,DWORD_PTR etc.. ) its an issue.

Sorry I so want this to be right, its singularly one of the things I've wanted for some time (you just got to it before me ;-) ) so I'm super excited by it.

From the other aspects we are getting there.

[ksyx]

Recognize long (len>=5) uppercased name taking a single line as return type
and fix the problem of adding newline below it:

void
afunc(int x) {
  return;
}
TYPENAME
func(int x, int y) {
  // ...
}

Related code: https://github.com/llvm/llvm-project/blob/1e512f022ad5c23dc4ef4e663f51d5d0bcbc7c69/clang/lib/Format/UnwrappedLineParser.cpp#L1712

[MyDeveloperDay]

Works nicely now... (running it over my own code base)

LGTM

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

120

Split into two asserts, then one would know which did not hold.

135

Shouldn't we set a type for such cases instead of repeating the detection code here?

136

As a Qt user who also wants to use your patch, please add a test for that case. ;)

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

143

Maybe really use HRESULT? People will know what that should be.

[ksyx]

clang/lib/Format/DefinitionBlockSeparator.cpp

135

Here I actually did a few more checks to limit the impact to the minimum but I am happy to do that if that's necessary.

136

For Q_OBJECT if used as pattern like this:

class X : ... {
  Q_OBJECT
public:
  // ...
}

I think this patch has no effect on this? The comment here is just a repetition of that in unwrapped parser

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

143

It actually does not matter what type name it is as long as it is an identifier with >=5 characters and all uppercased

[ksyx]

• Apply suggestions from clangfmt
• Split assertion conditions

[ksyx]

Apply suggestion from clangfmt

[curdeius]

In D117520#3251425, @MyDeveloperDay wrote:

For

class Test
{
public:
    static void foo()
    {
    }
};

$ clang-format -n test.cxx
test.cxx:3:8: warning: code should be clang-formatted [-Wclang-format-violations
]
public:
       ^

$ clang-format --output-replacements-xml test.cxx
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='20' length='5'>&#10;    </replacement>
</replacements>

but when I run it nothing changes

$ clang-format test.cxx
class Test
{
public:
    static void foo()
    {
    }
};

And no character change

@MyDeveloperDay, @ksyx
Is there a test for this?
If no, how can we test it? It happens only with --dry-run, right?

[MyDeveloperDay]

I think its been fixed, in the last but one diff.

Generally speaking we simply cannot have --output-replacements-xml , -n or --dry-run show replacements that amount to nothing because this is the mechanism by which 100s of repos fail commits thinking their is a clang-format issue.

Last revision I tried these issues seem to have disappeared

[ksyx]

Add unit test for formatting conflict

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

136

Which does indicate even more, that we shouldn't copy the stuff, but let the UnwrappedLineParser mark it as that, whatever that is.
What's your opinion @MyDeveloperDay ?

[curdeius]

In D117520#3258454, @MyDeveloperDay wrote:

I think its been fixed, in the last but one diff.

Generally speaking we simply cannot have --output-replacements-xml , -n or --dry-run show replacements that amount to nothing because this is the mechanism by which 100s of repos fail commits thinking their is a clang-format issue.

Last revision I tried these issues seem to have disappeared

I understood that it was fixed but I'd really like to have a test case that reproduces it.
We might have the same problem somewhere else, no?
Can we have a part of verifyFormat that runs with DryRun=true and checks the replacements?

[ksyx]

In D117520#3260443, @curdeius wrote:

In D117520#3258454, @MyDeveloperDay wrote:

I think its been fixed, in the last but one diff.

Generally speaking we simply cannot have --output-replacements-xml , -n or --dry-run show replacements that amount to nothing because this is the mechanism by which 100s of repos fail commits thinking their is a clang-format issue.

Last revision I tried these issues seem to have disappeared

I understood that it was fixed but I'd really like to have a test case that reproduces it.
We might have the same problem somewhere else, no?
Can we have a part of verifyFormat that runs with DryRun=true and checks the replacements?

Yes I see your point so I added one in the last diff :)

[curdeius]

I'd like to see the refactor of the parts repeated from UnwrappedLineParser.
No comments otherwise.

clang/lib/Format/DefinitionBlockSeparator.cpp

135

Shouldn't we set a type for such cases instead of repeating the detection code here?

👍

[ksyx]

Add token type FunctionLikeOrFreestandingMacro and use it to replace duplicated check with UnwrappedLineParser

[HazardyKnusperkeks]

clang/lib/Format/DefinitionBlockSeparator.cpp

135

Thanks! Shouldn't we drop are radically change this comment?

clang/lib/Format/UnwrappedLineParser.cpp

1686 ↗

(On Diff #402006)

This is more consistent with the rest of the code base.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

44–46

This is from Debugging?

143

That I know, but HRESULT would look familiar to at least some people.

[ksyx]

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

44–46

I suppose it is from rebasing.

[ksyx]

Apply review suggestions of renaming.

clang/lib/Format/DefinitionBlockSeparator.cpp

135

I've checked this comment but found nothing to change, since it has nothing to do with the duplicated check but just referring to where created this problem?

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp

143

This makes sense! Thanks for the suggestion.

[MyDeveloperDay]

maybe slightly related https://github.com/llvm/llvm-project/issues/53183 in that this is also impacted by the 5 character TT_FunctionLikeOrFreestandingMacro

[ksyx]

In D117520#3263403, @MyDeveloperDay wrote:

maybe slightly related https://github.com/llvm/llvm-project/issues/53183 in that this is also impacted by the 5 character TT_FunctionLikeOrFreestandingMacro

Yes this patch is just marking this type of recognition but surely it requires other changes to the formatter.

By the way are there any improvements needed for this patch? Thanks.

[HazardyKnusperkeks]

Just this small detail. :)

clang/lib/Format/DefinitionBlockSeparator.cpp

131

[curdeius]

LGTM.

clang/lib/Format/DefinitionBlockSeparator.cpp

141–145

Please merge these two ifs into one.

clang/lib/Format/DefinitionBlockSeparator.h

public:
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override;
 
 private:
   void separateBlocks(SmallVectorImpl<AnnotatedLine *> &Lines,
-                      tooling::Replacements &Result);
+                      tooling::Replacements &Result, FormatTokenLexer &Tokens);
 };
 } // namespace format
 } // namespace clang
 
 #endif

clang/lib/Format/DefinitionBlockSeparator.cpp

 namespace clang {
 namespace format {
 std::pair<tooling::Replacements, unsigned> DefinitionBlockSeparator::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
   assert(Style.SeparateDefinitionBlocks != FormatStyle::SDS_Leave);
   AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
   tooling::Replacements Result;
-  separateBlocks(AnnotatedLines, Result);
+  separateBlocks(AnnotatedLines, Result, Tokens);
   return {Result, 0};
 }
 
 void DefinitionBlockSeparator::separateBlocks(
-    SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result) {
+    SmallVectorImpl<AnnotatedLine *> &Lines, tooling::Replacements &Result,
+    FormatTokenLexer &Tokens) {
   const bool IsNeverStyle =
       Style.SeparateDefinitionBlocks == FormatStyle::SDS_Never;
-  auto LikelyDefinition = [this](const AnnotatedLine *Line) {
+  const AdditionalKeywords &ExtraKeywords = Tokens.getKeywords();
+  auto LikelyDefinition = [this, ExtraKeywords](const AnnotatedLine *Line,
+                                                bool ExcludeEnum = 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"))
+      if (CurrentToken->isOneOf(tok::kw_class, tok::kw_struct) ||
+          (Style.isJavaScript() && CurrentToken->is(ExtraKeywords.kw_function)))
+        return true;
+      if (!ExcludeEnum && CurrentToken->is(tok::kw_enum))
         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) {
     const auto &CurrentLine = Lines[I];
     if (CurrentLine->InPPDirective)
       continue;
     FormatToken *TargetToken = nullptr;
     AnnotatedLine *TargetLine;
     auto OpeningLineIndex = CurrentLine->MatchingOpeningBlockLineIndex;
     AnnotatedLine *OpeningLine = nullptr;
+    const auto IsAccessSpecifierToken = [](const FormatToken *Token) {
+      return Token->isAccessSpecifier() || Token->isObjCAccessSpecifier();
+    };
     const auto InsertReplacement = [&](const int NewlineToInsert) {
       assert(TargetLine);
       assert(TargetToken);
 
       // Do not handle EOF newlines.
-      if (TargetToken->is(tok::eof) && NewlineToInsert > 0)
+      if (TargetToken->is(tok::eof))
+        return;
+      if (IsAccessSpecifierToken(TargetToken) ||
+          (OpeningLineIndex > 0 &&
+           IsAccessSpecifierToken(Lines[OpeningLineIndex - 1]->First)))
         return;
       if (!TargetLine->Affected)
         return;
       Whitespaces.replaceWhitespace(*TargetToken, NewlineToInsert,
-                                    TargetToken->SpacesRequiredBefore - 1,
-                                    TargetToken->StartsColumn);
+                                    TargetToken->OriginalColumn,
+                                    TargetToken->OriginalColumn);
     };
     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() ||
              IsPPConditional(OpeningLineIndex - 1);
     };
-    const auto HasEnumOnLine = [CurrentLine]() {
+    const auto HasEnumOnLine = [&]() {
       FormatToken *CurrentToken = CurrentLine->First;
+      bool FoundEnumKeyword = false;
       while (CurrentToken) {
         if (CurrentToken->is(tok::kw_enum))
+          FoundEnumKeyword = true;
+        else if (FoundEnumKeyword && CurrentToken->is(tok::l_brace))
           return true;
         CurrentToken = CurrentToken->Next;
       }
-      return false;
+      return FoundEnumKeyword && I + 1 < Lines.size() &&
+             Lines[I + 1]->First->is(tok::l_brace);
     };
 
     bool IsDefBlock = false;
     const auto MayPrecedeDefinition = [&](const int Direction = -1) {
+      assert(Direction >= -1);
+      assert(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 (LikelyDefinition(OperateLine))
+        return false;
+
+      if (OperateLine->First->is(tok::comment))
+        return true;
+
+      // A single line identifier that is not in the last line
+      if (OperateLine->First->is(tok::identifier) &&
+          OperateLine->First == OperateLine->Last &&
+          OperateIndex + 1 < Lines.size()) {
+        // UnwrappedLineParser's recognition of free-standing macro like
+        // Q_OBJECT may also recognize some uppercased type names that may be
+        // used as return type as that kind of macros, which is a bit hard to
+        // distinguish one from another purely from token patterns. Here, we
+        // try not to add new lines below those identifiers.
+        AnnotatedLine *NextLine = Lines[OperateIndex + 1];
+        if (NextLine->MightBeFunctionDecl &&
+            NextLine->mightBeFunctionDefinition()) {
+          if (NextLine->First->NewlinesBefore == 1) {
+            const llvm::StringRef &Text = OperateLine->First->TokenText;
+            if (Text.size() >= 5 && Text.upper() == Text)
+              return true;
+          }
+        }
+      }
+
+      if ((Style.isCSharp() && OperateLine->First->is(TT_AttributeSquare)))
+        return true;
+      return false;
     };
 
-    if (HasEnumOnLine()) {
+    if (HasEnumOnLine() &&
+        !LikelyDefinition(CurrentLine, /*ExcludeEnum=*/true)) {
       // We have no scope opening/closing information for enum.
       IsDefBlock = true;
       OpeningLineIndex = I;
       while (OpeningLineIndex > 0 && MayPrecedeDefinition())
         --OpeningLineIndex;
       OpeningLine = Lines[OpeningLineIndex];
       TargetLine = OpeningLine;
       TargetToken = TargetLine->First;
       if (!FollowingOtherOpening())
         InsertReplacement(NewlineCount);
       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->is(tok::l_brace);
+      // Handling the case that opening brace has its own line, with checking
+      // whether the last line already had an opening brace to guard against
+      // misrecognition.
+      if (OpeningLineIndex > 0 &&
+          Lines[OpeningLineIndex]->First->is(tok::l_brace) &&
+          Lines[OpeningLineIndex - 1]->Last->isNot(tok::l_brace))
+        --OpeningLineIndex;
       OpeningLine = Lines[OpeningLineIndex];
       // Closing a function definition.
       if (LikelyDefinition(OpeningLine)) {
         IsDefBlock = true;
         while (OpeningLineIndex > 0 && MayPrecedeDefinition())
           --OpeningLineIndex;
         OpeningLine = Lines[OpeningLineIndex];
         TargetLine = OpeningLine;
if (IsDefBlock && I + 1 < Lines.size()) {
       // definition.
       if (!TargetToken->closesScope() && !IsPPConditional(OpeningLineIndex)) {
         // Check whether current line may precede a definition line.
         while (OpeningLineIndex + 1 < Lines.size() &&
                MayPrecedeDefinition(/*Direction=*/0))
           ++OpeningLineIndex;
         TargetLine = Lines[OpeningLineIndex];
         if (!LikelyDefinition(TargetLine)) {
+          OpeningLineIndex = I + 1;
           TargetLine = Lines[I + 1];
           TargetToken = TargetLine->First;
           InsertReplacement(NewlineCount);
         }
-      } else if (IsNeverStyle) {
-        TargetLine = Lines[I + 1];
-        TargetToken = TargetLine->First;
-        InsertReplacement(OpeningLineIndex != 0);
-      }
+      } else if (IsNeverStyle)
+        InsertReplacement(/*NewlineToInsert=*/1);
     }
   }
   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

protected:
   static std::string
   separateDefinitionBlocks(llvm::StringRef Code,
                            const FormatStyle &Style = getLLVMStyle()) {
     return separateDefinitionBlocks(
         Code,
         /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
   }
 
   static void verifyFormat(llvm::StringRef Code,
                            const FormatStyle &Style = getLLVMStyle(),
                            llvm::StringRef ExpectedCode = "") {
     bool HasOriginalCode = true;
     if (ExpectedCode == "") {
       ExpectedCode = Code;
       HasOriginalCode = false;
     }
 
     FormatStyle InverseStyle = Style;
     if (Style.SeparateDefinitionBlocks == FormatStyle::SDS_Always)
verifyFormat("namespace foo {\n"
                "int j, k;\n"
                "}",
                Style);
 
   verifyFormat("enum Foo { FOO, BAR };\n"
                "\n"
                "enum Bar { FOOBAR, BARFOO };\n",
                Style);
+
+  FormatStyle BreakAfterReturnTypeStyle = Style;
+  BreakAfterReturnTypeStyle.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;
+  // Test uppercased long typename
+  verifyFormat("class Foo {\n"
+               "  void\n"
+               "  Bar(int t, int p) {\n"
+               "    int r = t + p;\n"
+               "    return r;\n"
+               "  }\n"
+               "\n"
+               "  LONGTYPENAME\n"
+               "  Foobar(int t, int p) {\n"
+               "    int r = t * p;\n"
+               "    return r;\n"
+               "  }\n"
+               "}\n",
+               BreakAfterReturnTypeStyle);
+}
+
+TEST_F(DefinitionBlockSeparatorTest, FormatConflict) {
+  FormatStyle Style = getLLVMStyle();
+  Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+  llvm::StringRef Code = "class Test {\n"
+                         "public:\n"
+                         "  static void foo() {\n"
+                         "    int t;\n"
+                         "    return 1;\n"
+                         "  }\n"
+                         "};";
+  std::vector<tooling::Range> Ranges = {1, tooling::Range(0, Code.size())};
+  EXPECT_EQ(reformat(Style, Code, Ranges, "<stdin>").size(), 0u);
+}
+
+TEST_F(DefinitionBlockSeparatorTest, CommentBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always;
+  std::string Prefix = "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 Suffix = "enum Bar { FOOBAR, BARFOO };\n"
+                       "\n"
+                       "/* Comment block in one line*/\n"
+                       "int bar3(int j, int k) {\n"
+                       "  // A comment\n"
+                       "  int r = j % k;\n"
+                       "  return r;\n"
+                       "}\n";
+  std::string CommentedCode = "/*\n"
+                              "int bar2(int j, int k) {\n"
+                              "  int r = j / k;\n"
+                              "  return r;\n"
+                              "}\n"
+                              "*/\n";
+  verifyFormat(removeEmptyLines(Prefix) + "\n" + CommentedCode + "\n" +
+                   removeEmptyLines(Suffix),
+               Style, Prefix + "\n" + CommentedCode + "\n" + Suffix);
+  verifyFormat(removeEmptyLines(Prefix) + "\n" + CommentedCode +
+                   removeEmptyLines(Suffix),
+               Style, Prefix + "\n" + CommentedCode + Suffix);
 }
 
 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"
TEST_F(DefinitionBlockSeparatorTest, UntouchBlockStartStyle) {
   };
 
   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);
+  auto TestKit = MakeUntouchTest("/* FOOBAR */\n"
+                                 "#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);
+  TestKit = MakeUntouchTest("/* FOOBAR */\n"
+                            "#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);
std::string Infix = "\n"
                       "// 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"
+                      "/*const*/ 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 r = j * k;\n"
+                      "    return r;\n"
+                      "  }\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"
+                      "int bar3(int j, int k, const enum Bar b) {\n"
                       "  // A comment\n"
                       "  int r = j % k;\n"
                       "  return r;\n"
                       "}\n";
   std::string Postfix = "\n"
                         "} // namespace\n"
                         "\n"
                         "namespace T {\n"
TEST_F(DefinitionBlockSeparatorTest, Never) {
   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"
+                        "/*const*/ 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 r = j * k;\n"
+                        "    return r;\n"
+                        "  }\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"
+                        "int bar3(int j, int k, const enum Bar b) {\n"
                         "  // A comment\n"
                         "  int r = j % k;\n"
                         "  return r;\n"
                         "}\n"
                         "} // namespace";
   verifyFormat(Prefix + "\n\n\n" + Postfix, Style,
                Prefix + removeEmptyLines(Postfix));
 }
verifyFormat("namespace NS\n"
                "  FOO,\n"
                "  BAR\n"
                "};\n"
                "\n"
                "/*\n"
                "test1\n"
                "test2\n"
                "*/\n"
-               "int foo(int i, int j)\n"
+               "/*const*/ 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 r = j * k;\n"
+               "    return r;\n"
+               "  }\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"
+               "int bar3(int j, int k, const enum Bar b)\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"
+                        "/*const*/ 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 r = j * k;\n"
+                        "    return r;\n"
+                        "  }\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"
+                        "int bar3(int j, int k, const enum Bar b) {\n"
                         "  // A comment\n"
                         "  int r = j % k;\n"
                         "  return r;\n"
                         "}\n"
                         "} // namespace";
   verifyFormat(LeaveAs, Style, LeaveAs);
 }