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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits