HazardyKnusperkeks 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)
----------------
thezbyg wrote:
> 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;
> };
> ```
> 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.
================
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 &&
----------------
thezbyg wrote:
> 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.
I would assume so. I was just irritated by the >>. Everything is fine here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93846/new/
https://reviews.llvm.org/D93846
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
