Page MenuHomePhabricator

[clang-format] PR16518 Add flag to suppress empty line insertion before access modifier
AcceptedPublic

Authored by thezbyg on Sun, Dec 27, 2:12 PM.

Details

Summary

Add new option called InsertEmptyLineBeforeAccessModifier. Empty line before access modifier is inserted if this option is set to true (which is the default value, because clang-format always inserts empty lines before access modifiers), otherwise empty lines are removed.

Fixes issue #16518.

Diff Detail

Event Timeline

thezbyg requested review of this revision.Sun, Dec 27, 2:12 PM
thezbyg created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, Dec 27, 2:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thank you for this patch, a few process issues.

clang/include/clang/Format/Format.h
58

you need to run clang/doc/tools/dump_style.py so that it regenerates the ClangFormatsStyleOptions.rst file

68

quite a mouthful... maybe just NewLineBeforeAccessModifier ?

clang/test/Format/access-modifiers.cpp
2

This is good to add, but could you also add a test in clang/unittests/Format/FormatTests.cpp

MyDeveloperDay retitled this revision from Add flag to suppress empty line insertion before access modifier to [clang-format] PR16518 Add flag to suppress empty line insertion before access modifier.Sun, Dec 27, 2:19 PM
MyDeveloperDay added a project: Restricted Project.

My 2 cents. Otherwise I agree with @MyDeveloperDay's comments.

clang/lib/Format/UnwrappedLineFormatter.cpp
1218–1247

Given that the two ifs handle the same construct, I think we should try to merge them to avoid duplication in conditions. Sth like:

if (PreviousLine && RootToken.isAccessSpecifier()) {
    if (... Style.Insert ... && ... ) {
    ...
    }
    else if ( (... !Style.Insert ... && ... ) {
    ...
    }
}
1219

Just thinking out loud, but shouldn't we just check that PreviousLine->Last->isNot(tok::l_brace)?
Could you please add a test with comments before access modifiers?

1248–1249

Please add tests with preprocessor directives *before* access modifiers too.

1249

Is it really needed to check the type of PreviousLine->Last?

curdeius edited the summary of this revision. (Show Details)Mon, Dec 28, 6:14 AM
HazardyKnusperkeks added inline comments.
clang/include/clang/Format/Format.h
68

A new line is always there, how about EmptyLineBeforeAccessModifier?

Apart from that, I would prefer an alphabetical sorting of the member, most of them are (not all I know...).

2862

Here it is sorted.

clang/lib/Format/Format.cpp
457

Here again not.

MyDeveloperDay added inline comments.Mon, Dec 28, 7:28 AM
clang/include/clang/Format/Format.h
68

Yes EmptyLine seems the better termininology

thezbyg updated this revision to Diff 313863.Mon, Dec 28, 9:14 AM

Option renamed to EmptyLineBeforeAccessModifier.
Placed new configuration member in correct place alphabetically.
Last token in previous line is no longer checked when EmptyLineBeforeAccessModifier is false.
Executed clang/doc/tools/dump_style.py to update ClangFormatsStyleOptions.rst.
Unit tests added to clang/unittests/Format/FormatTests.cpp.

MyDeveloperDay added inline comments.Mon, Dec 28, 9:25 AM
clang/unittests/Format/FormatTest.cpp
8891–8903

if you use verifyFormat it will check what happens when it messes the code up to ensure its stable

thezbyg marked 10 inline comments as done.Mon, Dec 28, 9:32 AM
thezbyg added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
1219

No previous line token check is necessary if it is acceptable to remove empty lines in situations where EmptyLineBeforeAccessModifier=true would not add empty line.

Without any additional tests formatting the following code with EmptyLineBeforeAccessModifier=true:

struct foo {
private:
  int i;
}

would not add any empty lines, but formatting this code with EmptyLineBeforeAccessModifier=false:

struct foo {

private:
  int i;
}

would remove empty line before modifier.

thezbyg added inline comments.Mon, Dec 28, 10:14 AM
clang/unittests/Format/FormatTest.cpp
8891–8903

After switching to verifyFormat all tests pass when checking C++ formatting, but some of the same tests fail in Objective-C++ check:
C style comment is attached to previous line:

      Expected: Expected.str()
      Which is: "struct foo {\n  /* comment */\nprivate:\n  int i;\n  int j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
      Which is: "struct foo { /* comment */\nprivate:\n  int i;\n  int j;\n}\n"
With diff:
@@ -1,4 +1,3 @@
-struct foo {
-  /* comment */
+struct foo { /* comment */
 private:
   int i;

Empty line before access modifier is removed:

      Expected: Expected.str()
      Which is: "struct foo {\n  int i;\n\nprivate:\n  int j;\n}\n"
To be equal to: format(test::messUp(Code), ObjCStyle)
      Which is: "struct foo {\n  int i;\nprivate:\n  int j;\n}\n"
With diff:
@@ -1,5 @@
 struct foo {
   int i;
-
 private:
   int j;

Looks like empty lines before modifiers are removed for Objective-C++ language. What should I do?

MyDeveloperDay added inline comments.Tue, Dec 29, 3:45 AM
clang/unittests/Format/FormatTest.cpp
8891–8903

please update the patch at least to user verifyFomat where you can.

Its likely means we are missing something

thezbyg updated this revision to Diff 313973.Tue, Dec 29, 9:05 AM

Switched to verifyFormat in most of the tests.
Rerun clang-format.

Nit.

clang/docs/ClangFormatStyleOptions.rst
1877

Full stop at the end of the phrase.

MyDeveloperDay added inline comments.Wed, Dec 30, 2:34 AM
clang/include/clang/Format/Format.h
1644

The full stop will go here then regenerate.

thezbyg updated this revision to Diff 314129.Wed, Dec 30, 10:04 AM
thezbyg marked 3 inline comments as done.

Added missing full stop.
Executed clang/doc/tools/dump_style.py to update ClangFormatStyleOptions.rst.

ok so this looks ok, but before we commit can we have a discussion about why you think it fails for the comment case?

MyDeveloperDay added inline comments.Thu, Dec 31, 7:11 AM
clang/unittests/Format/FormatTest.cpp
8891

any reason this can't be verifyFormat?

8912

verifyFormat? why not

thezbyg marked 2 inline comments as done.Thu, Dec 31, 12:07 PM

After some updating, rebuilding and searching for differences in Objective-C++ formatting, I managed to find where the problem with these failing tests is. In _verifyFormat function C++ code formatting is tested for stability like this:

EXPECT_EQ(Expected.str(), format(Expected, Style));
EXPECT_EQ(Expected.str(), format(Code, Style));

, but Objective-C++ test, in the same function, looks like this:

EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));

test::messUp function removes all newlines and indentation, so test code:

struct foo {
  int i;

private:
  int j;
}

turns into:

struct foo { int i; private: int j; }

After running format on this code, we get incorrect result:

struct foo {
  int i;
private:
  int j;
}

Running format again would produce the correct output:

struct foo {
  int i;

private:
  int j;
}

So it seems that insertion of empty line fails when access modifier is in the same line as previous tokens. Unmodified clang-format produces the same output. As this behavior is not related to the changes in my patch, should we attempt to fix it here, or a separate bug report would be preferred?

The situation with tests containing comments is similar, because test::messUp single line output is formatted into:

struct foo { /* comment */
private:
  int i;
  int j;
}

which is not the same as:

struct foo {
  /* comment */
private:
  int i;
  int j;
}

In my opinion, Objective-C++ test::messUp test incorrectly expects any code collapsed into a single line to format into the same code as formatted original code.

When access modifier is in the same line with previous tokens, UnwrappedLineFormatter::formatFirstToken is called with RootToken.NewlinesBefore == 0, but empty line is only inserted if RootToken.NewlinesBefore == 1. The following change fixes this and passes all tests except the ones with comment:

   if (PreviousLine && RootToken.isAccessSpecifier()) {
     if (Style.EmptyLineBeforeAccessModifier &&
         PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
-        RootToken.NewlinesBefore == 1)
-      ++Newlines;
+        RootToken.NewlinesBefore <= 1)
+      Newlines = 2;
     else if (!Style.EmptyLineBeforeAccessModifier &&
              RootToken.NewlinesBefore > 1)
       Newlines = 1;
   }
MyDeveloperDay added inline comments.Fri, Jan 1, 10:15 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
1222

maybe I don't understand the logic but why is this r_brace and shouldn't we be looking at the "Last Non Comment" token?

thezbyg marked an inline comment as done.Mon, Jan 4, 9:31 AM
thezbyg added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
1222

I think that the logic is to only add empty line when access modifier is after member function body (r_brace indicates this) or declaration of field/function. If this check is changed to look at "last non comment" token, then empty line will be inserted in code like this:

struct Foo {
  int i;
  //comment
private:
  int j;
}
clang/lib/Format/UnwrappedLineFormatter.cpp
1222

But with the given Name it should add an empty line there! Otherwise you should use an enum and not a bool to control wether a comment should suppress the empty line or not. Also you should make the exception(s) clear with unit tests.

1223–1224

A tab?

clang/unittests/Format/FormatTest.cpp
8959–8972

Just curios, any reason why the struct is repeated? I don't seem to notice a difference.

And by the way, you are missing the ; at the end of the definition, I don't know if that affects clang-format.

MyDeveloperDay added inline comments.Tue, Jan 5, 1:27 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
1223–1224

My experience is that this doesn't mean a tab but just the what phabricator shows a change in whitespace

it is now 2 levels of if scope indented not 1 so I think it should be moved over abit

thezbyg marked an inline comment as done.Tue, Jan 5, 12:10 PM
thezbyg added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
1222

Current clang-format behavior of access modifier separation was added in commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is called SeparatesLogicalBlocks. So could we simply rename this new option to SeparateLogicalBlocks and leave the bool type? Option description would contain code examples and further explanation what logical block means. Setting SeparateLogicalBlocks option to false would disable empty line insertion, but would not remove existing empty lines.

Is option name EmptyLineBeforeAccessModifier acceptable if enum is used? And how should I name enum values? One enum value could be called Never, but other must indicate that empty line is only inserted when access modifier is after member function body or field/function/struct declaration.

I think that you incorrectly assumed from my previous comment, that only comments suppress empty line insertion. No empty line is inserted in all following code examples:

struct Foo {
private:
  int j;
};
struct Foo {
private:
public:
  int j;
};
struct Foo {
#ifdef FOO
private:
#endif
  int j;
};
1223–1224

Yes, there is no tab in submitted patch, only 6 spaces.

Is this indented incorrectly?

if (PreviousLine && RootToken.isAccessSpecifier()) {
  if (Style.EmptyLineBeforeAccessModifier &&
      PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
      RootToken.NewlinesBefore == 1)
    ++Newlines;
  else if (!Style.EmptyLineBeforeAccessModifier &&
           RootToken.NewlinesBefore > 1)
    Newlines = 1;
}

I always run git clang-format-11 HEAD~1 before generating patch file and uploading it here.

clang/unittests/Format/FormatTest.cpp
8959–8972

Some of the tests have equal expected and code values. I will update them to single parameter verifyFormat().

clang-format does not seem to care about missing ;, but I will add them as all other tests have them.

clang/lib/Format/UnwrappedLineFormatter.cpp
1222

Current clang-format behavior of access modifier separation was added in commit (fd433365e08da024ddc99dd0d7bce8e89d8c5469) and the test function is called SeparatesLogicalBlocks. So could we simply rename this new option to SeparateLogicalBlocks and leave the bool type? Option description would contain code examples and further explanation what logical block means. Setting SeparateLogicalBlocks option to false would disable empty line insertion, but would not remove existing empty lines.

Is option name EmptyLineBeforeAccessModifier acceptable if enum is used? And how should I name enum values? One enum value could be called Never, but other must indicate that empty line is only inserted when access modifier is after member function body or field/function/struct declaration.

That now depends on what you want (at least for me), if the name stays EmptyLineBeforeAccessModifier and it stays a bool it should nearly always add a space (most likely not directly after the {. I think the name would still fit for an enum with values like Always, Never, DontModify, and similar.

If you change the name it can stay a bool, but then needs an explanation with examples what the option really means.

1223–1224

I would assume so. I was just irritated by the >>. Everything is fine here.

thezbyg updated this revision to Diff 314962.Wed, Jan 6, 1:01 PM

Switched EmptyLineBeforeAccessModifierStyle option from bool to enum.
EmptyLineBeforeAccessModifierStyle option can now have one of four values: Never, DontModify, LogicalBlock, Always.
Never removes all empty lines before access modifiers.
DontModify keeps existing empty lines.
LogicalBlock adds empty line only when access modifier starts a new logical block (current clang-format behavior).
Always adds empty line before all access modifiers except when access modifier is at the start of class/struct definition.

Updated tests to test all four option values. When testing DontModify option, some of the tests use EXPECT_EQ() instead of verifyFormat(). This is because empty lines are not preserved in verifyFormat() due to test::messUp().

I think your base for the diff is wrong, there are many "added" things which are already in clang-format. Could you update the diff?

thezbyg updated this revision to Diff 315578.Sat, Jan 9, 1:20 AM

Diff updated. Previous diff was generated after rebase, and Phabricator change preview did not show any unrelated changes, so I thought that everything is fine.

Diff updated. Previous diff was generated after rebase, and Phabricator change preview did not show any unrelated changes, so I thought that everything is fine.

Now your diff removes the stuff?
You should rebase your change on master or main, (ideally) have only one commit on top of it, and than diff HEAD^1.

thezbyg updated this revision to Diff 316446.Wed, Jan 13, 10:24 AM

Rebased changes on master.

clang/lib/Format/Format.cpp
235

Other places use Leave I assume that means the same as DontModify?

Apart from that last naming issue looks good to me.

clang/lib/Format/Format.cpp
235

If that's the case, we should stick with it. Although I personally find DontModify better.

thezbyg updated this revision to Diff 316511.Wed, Jan 13, 2:03 PM

Renamed DontModify enum to Leave.

thezbyg marked 10 inline comments as done.Wed, Jan 13, 2:05 PM
This revision is now accepted and ready to land.Wed, Jan 13, 2:18 PM
curdeius accepted this revision.Thu, Jan 14, 4:50 AM

One last nit, otherwise LGTM.

clang/unittests/Format/FormatTest.cpp
8891

For the ease of understanding that you test LogicalBlock in this first part, I'd add:

FormatStyle Style = getLLVMStyle();
EXPECT_EQ(Style.EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_LogicalBlock);

and use this Style in verifyFormat.

thezbyg updated this revision to Diff 316660.Thu, Jan 14, 7:48 AM

Improved default style tests.

thezbyg marked an inline comment as done.Thu, Jan 14, 7:48 AM