[PATCH] D34225: [clang-format] Teach clang-format how to handle C++ coroutines

[Daniel Jasper via Phabricator via cfe-commits]

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