thezbyg marked an inline comment as done.
thezbyg added inline comments.
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1221
+ if (Style.EmptyLineBeforeAccessModifier &&
+ PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+ RootToken.NewlinesBefore == 1)
----------------
HazardyKnusperkeks wrote:
> thezbyg wrote:
> > MyDeveloperDay wrote:
> > > 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?
> > 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;
> > }
> > ```
> 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.
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;
};
```
================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1222-1223
+ PreviousLine->Last->isOneOf(tok::semi, tok::r_brace) &&
+ RootToken.NewlinesBefore == 1)
+ ++Newlines;
+ else if (!Style.EmptyLineBeforeAccessModifier &&
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > A tab?
> 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
>
>
>
>
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.
================
Comment at: clang/unittests/Format/FormatTest.cpp:8612-8625
+ verifyFormat("struct foo {\n"
+ "#ifdef FOO\n"
+ "private:\n"
+ "#endif\n"
+ " int i;\n"
+ " int j;\n"
+ "}\n",
----------------
HazardyKnusperkeks wrote:
> 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.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93846/new/
https://reviews.llvm.org/D93846
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
