- Added an option where to put the requires clauses.
- Renamed IndentRequires to IndentRequiresClause.
- Changed BreakBeforeConceptDeclaration from bool to an enum.
Fixes https://llvm.org/PR32165, and https://llvm.org/PR52401.
Differential D113319
[clang-format] Improve require and concept handling HazardyKnusperkeks on Nov 5 2021, 2:03 PM. Authored by
Details
Fixes https://llvm.org/PR32165, and https://llvm.org/PR52401.
Diff Detail
Event TimelineComment Actions Or am I mistaken and we want to be able to control the line break? Also I would break before struct, class, union, but I would have needed to change the test, and I know that we at least need to talk about in in front. :) template<typename T> require X<T> class ... vs. template<typename T> require X<T> class ...
Comment Actions I feel anything you do here would be an improvement. its probably worth looking at recent issues logged around this issue at the same time https://bugs.llvm.org/show_bug.cgi?id=52401 When I added this support its was basically to prevent it from mascaraing concepts, take my implementation with a grain of salt if you think it needs changing please feel free to propose it. Comment Actions I will take a look at the issues and incorporate them. I think I will add a few new options. Comment Actions Update on my work in progress. The new test cases are all passed. The old ones are only kept to pick some lines from them. I would like to hear some feedback while I finish the rest, mostly writing tests. Comment Actions Still WIP, not marking as changes planned, so that it pops up on your list and you can give me feedback. :)
Comment Actions Thanks for working on this! I love to see better requires support in clang-format. I didn't do a real review, just some drive-by comments.
Comment Actions This took a lot of work and debugging. I'm confident that we can now format every sane use of requires clauses, expressions, and concept declarations. And even some I wouldn't call sane. Decided to drop https://llvm.org/PR32166 since in this approach the book keeping to bring the indention back on track after the clause would be too much burden on everyone not using it. Comment Actions Woow, that's a mountain of work!
Comment Actions @HazardyKnusperkeks I think this is tremendous, I think this looks and feels like a much more thorough approach to formatting concepts. Thank you so much for taking this on and for adding SO many unit tests that gives us lots of good examples, I'm probably not doing as an in-depth review as others because my own team aren't able to use a compiler on all platforms that allow us to use concepts and so my skill level around them isn't as great because I don't use them day to day. But I'm going to give this a LGTM and when the others are happy I'm more than happy. My personal opinion is when reviews get this big, and we are making forward progress then I'm ok to "BANK!" what we have. My fear for large reviews is if we get a regression in some other behaviour and we have to say "What changed" and the response is "Everything Changed" then finding out why becomes so much harder. So from my perspective, I'd like to see this landed and any additional issues worked out in separate reviews. Comment Actions I can just say: ship it! Thanks for all the work!
Comment Actions It appears that after this patch clang-format started breaking up requires in javascript, e.g.: // before function f() { var requires = {}; } // after function f() { var requires = {}; } Could we restrict the requires logic only to (obj)C++? (normally I'd go and come up with a patch for cases like this, but this patch is quite large and it's not immediately clear where's the best place for this restriction). Comment Actions It appears that this causes a regression by adding a space in-between pointer dereferences *p -> * p in some cases, e.g. formatting this with llvm style: // before void f() { while (p < a && *p == 'a') p++; } // after void f() { while (p < a && * p == 'a') p++; } Comment Actions * in *p is indeed annotated as a binary operator now: clang-format-main-20220110-5ff916ab72b26e667bd5d2e4a762650ba479c781--style=file --debug-only=format-token-annotator test.cpp AnnotatedTokens(L=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=while L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x2f4bf80a6a0 Text='while' M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=l_paren L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='(' M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=identifier L=8 PPK=2 FakeLParens=10/5/ FakeRParens=0 II=0x2f4bf806fd8 Text='p' M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=50 Name=less L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<' M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=50 Name=identifier L=12 PPK=2 FakeLParens= FakeRParens=1 II=0x2f4bf807008 Text='q' M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=45 Name=ampamp L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&' M=0 C=1 T=UnaryOperator S=1 F=0 B=0 BK=0 P=45 Name=star L=17 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x0 Text='*' M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=100 Name=identifier L=18 PPK=2 FakeLParens= FakeRParens=2 II=0x2f4bf806fd8 Text='p' M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')' ---- AnnotatedTokens(L=1): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=semi L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';' ---- AnnotatedTokens(L=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=eof L=0 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='' ---- while (p < q && *p) ; whereas it was (correctly) a unary operator: clang-format-main-20220222-e0219872--style=file --debug-only=format-token-annotator test.cpp AnnotatedTokens(L=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=while L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x23639666fa0 Text='while' M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=l_paren L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='(' M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=identifier L=8 PPK=2 FakeLParens=10/5/ FakeRParens=0 II=0x236396a30a0 Text='p' M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=50 Name=less L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<' M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=50 Name=identifier L=12 PPK=2 FakeLParens= FakeRParens=1 II=0x236396a30d0 Text='q' M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=45 Name=ampamp L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&' M=0 C=1 T=BinaryOperator S=1 F=0 B=0 BK=0 P=45 Name=star L=17 PPK=2 FakeLParens=14/ FakeRParens=0 II=0x0 Text='*' M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=54 Name=identifier L=19 PPK=2 FakeLParens= FakeRParens=2 II=0x236396a30a0 Text='p' M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')' ---- AnnotatedTokens(L=1): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=semi L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';' ---- AnnotatedTokens(L=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=eof L=0 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='' ---- while (p < q && * p) ; Comment Actions That means someone guesses this is a binary op and guesses wrong. I try to take a look at it. Comment Actions
Yeah, I just found out that it is set in determineStarAmpUsage here: if (PrevToken->Tok.isLiteral() || PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true, tok::kw_false, tok::r_brace) || NextToken->Tok.isLiteral() || NextToken->isOneOf(tok::kw_true, tok::kw_false) || NextToken->isUnaryOperator() || // If we know we're in a template argument, there are no named // declarations. Thus, having an identifier on the right-hand side // indicates a binary operator. (InTemplateArgument && NextToken->Tok.isAnyIdentifier())) return TT_BinaryOperator; Strangely, InTemplateArgument is true here, but not sure how that happens, as it wasn't something you changed directly. Comment Actions @HazardyKnusperkeks, forget my previous comment, I think I found what changed. Comment Actions Yep, confirmed, but I don't know how to fix it. [ FAILED ] FormatTest.Concepts [ FAILED ] FormatTest.RequiresClauses [ FAILED ] TokenAnnotatorTest.UnderstandsRequiresClausesAndConcepts Hopefully you have some idea how to fix it... Oh, BTW, here's the test for the regression that Krasimir found: TEST_F(TokenAnnotatorTest, UnderstandsLoops) { auto Tokens = annotate("while (p < q && *p == 0) {}"); EXPECT_EQ(Tokens.size(), 14u) << Tokens; EXPECT_TOKEN(Tokens[6], tok::star, TT_UnaryOperator); } |
This one has cost me a lot of time, should we rethink this?