[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
djasper added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2267 + return true; +if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && +Left.Previous && Left.Previous->is(tok::kw_operator)) EricWF wrote: > djasper wrote: > > I know that the split between spaceRrequiredBefore and spaceRequiredBetween > > is generally bad. However, here you seem to be testing the exact same thing > > that is then retested in spaceRequiredBetween. I'd prefer the logic to be > > in one place only. Which tests break if you remove the changes to > > spaceRequiredBetween? > > Which tests break if you remove the changes to spaceRequiredBetween? > > Not sure I have a test, I was probably mistaken adding this. > > @djasper Assuming only one set of changes is needed, would they be better in > `spaceRequiredBetween` or `spaceRequiredBefore`? I don't know that I have a good answer to that. But I'd leave it here in the C++ language specific part. https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
EricWF marked an inline comment as done. EricWF added a comment. @alexfh Thanks for correcting the reviewers. I had no idea who to add so I used something like `git-suggest-reviewers`. Comment at: lib/Format/Format.cpp:1958 LangOpts.DeclSpecKeyword = 1; // To get __declspec. + LangOpts.CoroutinesTS = 1; return LangOpts; djasper wrote: > Can we change this only if Style.isCpp()? We should probably do that for > other things, too, but as we are adding more keywords here, lets not do that > for Java/JS/Proto. SGTM. I'll make the change. Comment at: lib/Format/TokenAnnotator.cpp:589 } + if (Style.isCpp()) +if (CurrentToken && CurrentToken->is(tok::kw_co_await)) djasper wrote: > if (Style.isCpp() && CurrentToken && ..) > next(); The `Style.isCpp()` should be unneeded now that the Coroutines TS keywords only parse when the style is Cpp. Comment at: lib/Format/TokenAnnotator.cpp:2267 + return true; +if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && +Left.Previous && Left.Previous->is(tok::kw_operator)) djasper wrote: > I know that the split between spaceRrequiredBefore and spaceRequiredBetween > is generally bad. However, here you seem to be testing the exact same thing > that is then retested in spaceRequiredBetween. I'd prefer the logic to be in > one place only. Which tests break if you remove the changes to > spaceRequiredBetween? > Which tests break if you remove the changes to spaceRequiredBetween? Not sure I have a test, I was probably mistaken adding this. @djasper Assuming only one set of changes is needed, would they be better in `spaceRequiredBetween` or `spaceRequiredBefore`? Comment at: lib/Format/UnwrappedLineParser.cpp:433 case tok::kw___try: + assert(!Tok->is(tok::kw_co_await)); if (!LBraceStack.empty() && LBraceStack.back()->BlockKind == BK_Unknown) djasper wrote: > EricWF wrote: > > This change is accidental. > What does that mean? Remove it? Sorry, I meant to remove it, I just couldn't when I left the comment as a reminder. https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
EricWF updated this revision to Diff 106135.
EricWF marked an inline comment as done.
EricWF added a comment.
- Address most review comments except those about `spaceBreakBetween` and
`spaceBreakBefore`.
https://reviews.llvm.org/D34225
Files:
lib/Format/Format.cpp
lib/Format/TokenAnnotator.cpp
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11085,6 +11085,40 @@
EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
}
+TEST_F(FormatTest, CoawaitForRangeCpp11) {
+ EXPECT_EQ("for co_await (auto x : range())\n ;",
+format("for co_await(auto x : range());"));
+}
+
+TEST_F(FormatTest, Coawait) {
+ EXPECT_EQ("int x = co_await foo();",
+format("int x = co_await foo();"));
+ EXPECT_EQ("int x = (co_await foo());",
+format("int x = (co_await foo());"));
+ EXPECT_EQ("co_await (42);",
+format("co_await(42);"));
+ EXPECT_EQ("void operator co_await(int);",
+format("void operator co_await (int);"));
+}
+
+TEST_F(FormatTest, Coyield) {
+ EXPECT_EQ("int x = co_yield foo();",
+format("int x = co_yield foo();"));
+ EXPECT_EQ("int x = (co_yield foo());",
+format("int x = (co_yield foo());"));
+ EXPECT_EQ("co_yield (42);",
+format("co_yield(42);"));
+}
+
+TEST_F(FormatTest, Coreturn) {
+ EXPECT_EQ("int x = co_return foo();",
+format("int x = co_return foo();"));
+ EXPECT_EQ("int x = (co_return foo());",
+format("int x = (co_return foo());"));
+ EXPECT_EQ("co_return (42);",
+format("co_return(42);"));
+}
+
} // end namespace
} // end namespace format
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1700,6 +1700,8 @@
if (Style.Language == FormatStyle::LK_JavaScript &&
FormatTok->is(Keywords.kw_await))
nextToken();
+ if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
+nextToken();
if (FormatTok->Tok.is(tok::l_paren))
parseParens();
if (FormatTok->Tok.is(tok::l_brace)) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -609,6 +609,8 @@
if (CurrentToken && CurrentToken->is(Keywords.kw_await))
next();
}
+ if (CurrentToken && CurrentToken->is(tok::kw_co_await))
+ next();
Contexts.back().ColonIsForRangeExpr = true;
next();
if (!parseParens())
@@ -2245,6 +2247,11 @@
if (Right.is(tok::l_paren)) {
if (Left.is(tok::r_paren) && Left.is(TT_AttributeParen))
return true;
+if (Left.is(tok::kw_co_await) && Left.Previous &&
+Left.Previous->is(tok::kw_operator))
+ return false;
+if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return))
+ return true;
return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
@@ -2292,6 +2299,18 @@
if (Style.isCpp()) {
if (Left.is(tok::kw_operator))
return Right.is(tok::coloncolon);
+// for await ( ...
+if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+Left.Previous && Left.Previous->is(tok::kw_for))
+ return true;
+if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+Left.Previous && Left.Previous->is(tok::kw_operator))
+ return false;
+if (Right.is(tok::star) && Left.is(tok::kw_co_yield))
+ return false;
+if (Right.isOneOf(tok::l_brace, tok::l_square) &&
+Left.isOneOf(tok::kw_co_yield, tok::kw_co_await))
+ return true;
} else if (Style.Language == FormatStyle::LK_Proto ||
Style.Language == FormatStyle::LK_TextProto) {
if (Right.is(tok::period) &&
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2001,6 +2001,7 @@
LangOpts.ObjC2 = 1;
LangOpts.MicrosoftExt = 1;// To get kw___try, kw___finally.
LangOpts.DeclSpecKeyword = 1; // To get __declspec.
+ LangOpts.CoroutinesTS = Style.isCpp();
return LangOpts;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
djasper added inline comments. Comment at: lib/Format/Format.cpp:1958 LangOpts.DeclSpecKeyword = 1; // To get __declspec. + LangOpts.CoroutinesTS = 1; return LangOpts; Can we change this only if Style.isCpp()? We should probably do that for other things, too, but as we are adding more keywords here, lets not do that for Java/JS/Proto. Comment at: lib/Format/TokenAnnotator.cpp:589 } + if (Style.isCpp()) +if (CurrentToken && CurrentToken->is(tok::kw_co_await)) if (Style.isCpp() && CurrentToken && ..) next(); Comment at: lib/Format/TokenAnnotator.cpp:2216 +if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return)) + return true; return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || Shouldn't this respect Style.SpaceBeforeParens? Comment at: lib/Format/TokenAnnotator.cpp:2267 + return true; +if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && +Left.Previous && Left.Previous->is(tok::kw_operator)) I know that the split between spaceRrequiredBefore and spaceRequiredBetween is generally bad. However, here you seem to be testing the exact same thing that is then retested in spaceRequiredBetween. I'd prefer the logic to be in one place only. Which tests break if you remove the changes to spaceRequiredBetween? Comment at: lib/Format/TokenAnnotator.cpp:2270 + return false; +if (Right.is(tok::star) && Left.is(tok::kw_co_yield)) + return false; Is this tested? Similar for all the other branches? Comment at: lib/Format/UnwrappedLineParser.cpp:433 case tok::kw___try: + assert(!Tok->is(tok::kw_co_await)); if (!LBraceStack.empty() && LBraceStack.back()->BlockKind == BK_Unknown) EricWF wrote: > This change is accidental. What does that mean? Remove it? https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
krasimir added a comment. Looks good to me too, but I actually don't know this code. Leaving to Daniel! https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
alexfh added a comment. This looks good overall, btw. But please wait for Krasimir or Daniel, since they actually know this code ;] https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
alexfh added a comment. Leaving only the most likely reviewers for this code. https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
EricWF added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:433 case tok::kw___try: + assert(!Tok->is(tok::kw_co_await)); if (!LBraceStack.empty() && LBraceStack.back()->BlockKind == BK_Unknown) This change is accidental. https://reviews.llvm.org/D34225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines
EricWF created this revision.
Herald added a subscriber: klimek.
This patch (1) enable parsing for coroutines by default and (2) teaches
clang-format how to properly handle them.
https://reviews.llvm.org/D34225
Files:
lib/Format/Format.cpp
lib/Format/TokenAnnotator.cpp
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10612,6 +10612,40 @@
EXPECT_EQ("auto c = u8'a';", format("auto c = u8'a';"));
}
+TEST_F(FormatTest, CoawaitForRangeCpp11) {
+ EXPECT_EQ("for co_await (auto x : range())\n ;",
+format("for co_await(auto x : range());"));
+}
+
+TEST_F(FormatTest, Coawait) {
+ EXPECT_EQ("int x = co_await foo();",
+format("int x = co_await foo();"));
+ EXPECT_EQ("int x = (co_await foo());",
+format("int x = (co_await foo());"));
+ EXPECT_EQ("co_await (42);",
+format("co_await(42);"));
+ EXPECT_EQ("void operator co_await(int);",
+format("void operator co_await (int);"));
+}
+
+TEST_F(FormatTest, Coyield) {
+ EXPECT_EQ("int x = co_yield foo();",
+format("int x = co_yield foo();"));
+ EXPECT_EQ("int x = (co_yield foo());",
+format("int x = (co_yield foo());"));
+ EXPECT_EQ("co_yield (42);",
+format("co_yield(42);"));
+}
+
+TEST_F(FormatTest, Coreturn) {
+ EXPECT_EQ("int x = co_return foo();",
+format("int x = co_return foo();"));
+ EXPECT_EQ("int x = (co_return foo());",
+format("int x = (co_return foo());"));
+ EXPECT_EQ("co_return (42);",
+format("co_return(42);"));
+}
+
} // end namespace
} // end namespace format
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -430,6 +430,7 @@
case tok::kw_switch:
case tok::kw_try:
case tok::kw___try:
+ assert(!Tok->is(tok::kw_co_await));
if (!LBraceStack.empty() && LBraceStack.back()->BlockKind == BK_Unknown)
LBraceStack.back()->BlockKind = BK_Block;
break;
@@ -1674,6 +1675,8 @@
if (Style.Language == FormatStyle::LK_JavaScript &&
FormatTok->is(Keywords.kw_await))
nextToken();
+ if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
+nextToken();
if (FormatTok->Tok.is(tok::l_paren))
parseParens();
if (FormatTok->Tok.is(tok::l_brace)) {
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -586,6 +586,9 @@
if (CurrentToken && CurrentToken->is(Keywords.kw_await))
next();
}
+ if (Style.isCpp())
+if (CurrentToken && CurrentToken->is(tok::kw_co_await))
+ next();
Contexts.back().ColonIsForRangeExpr = true;
next();
if (!parseParens())
@@ -2206,6 +2209,11 @@
if (Right.is(tok::l_paren)) {
if (Left.is(tok::r_paren) && Left.is(TT_AttributeParen))
return true;
+if (Left.is(tok::kw_co_await) && Left.Previous &&
+Left.Previous->is(tok::kw_operator))
+ return false;
+if (Left.isOneOf(tok::kw_co_await, tok::kw_co_yield, tok::kw_co_return))
+ return true;
return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
@@ -2252,6 +2260,18 @@
if (Style.isCpp()) {
if (Left.is(tok::kw_operator))
return Right.is(tok::coloncolon);
+// for await ( ...
+if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+Left.Previous && Left.Previous->is(tok::kw_for))
+ return true;
+if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) &&
+Left.Previous && Left.Previous->is(tok::kw_operator))
+ return false;
+if (Right.is(tok::star) && Left.is(tok::kw_co_yield))
+ return false;
+if (Right.isOneOf(tok::l_brace, tok::l_square) &&
+Left.isOneOf(tok::kw_co_yield, tok::kw_co_await))
+ return true;
} else if (Style.Language == FormatStyle::LK_Proto) {
if (Right.is(tok::period) &&
Left.isOneOf(Keywords.kw_optional, Keywords.kw_required,
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1955,6 +1955,7 @@
LangOpts.ObjC2 = 1;
LangOpts.MicrosoftExt = 1;// To get kw___try, kw___finally.
LangOpts.DeclSpecKeyword = 1; // To get __declspec.
+ LangOpts.CoroutinesTS = 1;
return LangOpts;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
