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

[Eric Fiselier via Phabricator via cfe-commits]

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