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