clang-format: Space and triple angle braces

[jpienaar]

Summary

clang-format changes "<<<" to "<< <" which is problematic for CUDA code. Change the behavior of clang-format to operate correctly with CUDA kernel launches without changing language options.

• No space required between "<<" and "<" in "<<<".
• No space required between identifier and "<<<". This avoids changing "f<<<" to "f <<<"
• No space required between ">" and "<<<". Avoids changing "f<t><<<" to "f<t> <<<".
• No space required between ">>>" and "("

Such that "f<param><<<1, 1>>>();" remains unchanged instead of being changed to "f<param> << <1, 1>>> ();"

[djasper]

We don't generally want to use the spacing in the input to matter for the output. Is this really important? What would be the downside of using language options?

Also, just controlling the space might not be sufficient as clang-format also might go ahead break between the tokens. Generally, it might be better to actually merge the tokens (as is done for the === operation in JavaScript), as these should be considered to be a single token. Alternatively, you need to at least control CanBreakBefore for these and add tests for BreakBeforeBinaryOperators set to All and None.

[jpienaar]

If CUDA language option is enabled then
A<A<A<int>>> a;
gets changed to
A < A < A < int >>> a;
as >>> at the end is no longer 3 tokens but 1. I could modify the
parseAngle (I believe) function to address this case though. The majority
of uses would be for regular C++, so enabling CUDA language option by
default would seem to benefit me but possibly causing others headaches, so
I went for a route where the opening triple angle bracket is retained as is
in as direct a manner as possible while affecting as few other code paths
as possible.

The importance is in the CUDA code generation side. foo<< <1,1>>>() would
not be recognized as a CUDAKernelCallExpr. So an input program that
compiled would not compile post formatting.

Would it be OK to merge the tokens in the following case?
friend std::ostream& operator << <T> (std::ostream&, const Self&);
That was pretty much the only other usage for three open angle brackets I
saw. Currently I think it would be merged, so that would not change but I
want to double check. If so, then I'll follow your suggestion and add those
tests thanks.

[jpienaar]

Version setting CUDA language option for comparison based on discussion earlier today.

The parseAngle function updated to treat >>> as 3 tokens. These are without the binary operator split tests as now both "<<<" and ">>>" are tokens. As the AST is not constructed, setting the CUDA language option should not have other side-effects here other than to recognize <<< and >>> as 1 token each.

[djasper]

lib/Format/TokenAnnotator.cpp

68

I really don't like such static variables. Also, now we have two such mechanisms (GreaterStashed and Count). Seems like the same mechanisms should be used.

69

In LLVM style, we don't allow one-liner ifs. Use clang-format ..

71–73

Do these lines actually change the behavior? Seems like the entire wrong place to do this.

1809

So, if we just set TT_TemplateOpener/Closer on these, is this really required?

unittests/Format/FormatTest.cpp

9616

nit: TripleAngleBrackets (one "p" and I don't think that these are "braces").

[jpienaar]

Removed the static variable and perform mergin in FormatTokenLexer. Also ran from clang-format -style llvm which changed some spacing.

[jpienaar]

The web ui is a bit confusing when I try to reply to comments on previous revision. So I'll reply here. Most of the previous comments probably don't apply since your pointer to GreaterStashed made me redo that part (thanks!). All the rules (except the last one about space between >>> and ( ) do effect the output, and the one about >>> and ( is probably fine. I didn't want to always merge greater, greater, greater into greatergreatergreater as then the template parsing logic needs to be modified, but if there is a lesslessless in the same instruction then that's a reasonably good signal that it is a kernel launch.

[jpienaar]

Redone following a simpler approach of splitting up << into < and <, and remerging only if not <<<. The formatting of kernel launch parameters then follows that of template instantiation, The previous approach could also have resulted in unmatched braces.

[djasper]

Could you generally upload diffs with the full file as context? That makes review here much easier.

lib/Format/Format.cpp

665

Wouldn't we want to fall back to merging in this case?

I think if the user formats the incomplete line "llvm::outs() <<", we should not rip apart the <<. Add a test for this.

674

Make this:

// Only merge if there currently is no whitespace between the two "<".

677

Indentation... Maybe use clang-format ;-).

679

I think this comment doesn't add information. Remove.

lib/Format/TokenAnnotator.cpp

2043

A few lines higher up, there is at test:

if (Left.is(tok::greater) && Right.is(tok::greater) &&
    Left.isNot(TT_TemplateCloser))
  return false;

Seems like we should combine those. Maybe simply remove the "Left.isNot(TT_TemplateCloser)"?

2044

Indentation.

[jpienaar]

Made the changes recommended. clang-format changed unrelated things in the file.

[djasper]

I think that this is basically good to go. Do you have commit access?

lib/Format/Format.cpp

665

Use early exit.

681

Maybe just use "auto" here.

[jpienaar]

Done. I've simplified around line 665 a bit too.

I have commit access and will commit in a couple of hours. Thanks.

lib/Format/Format.cpp

   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.CPlusPlus14 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
+  LangOpts.CUDA = Style.Language == FormatStyle::LK_Cpp ? 1 : 0;
   LangOpts.LineComment = 1;
   bool AlternativeOperators = Style.Language != FormatStyle::LK_JavaScript &&
                               Style.Language != FormatStyle::LK_Java;

lib/Format/TokenAnnotator.cpp

         next();
         return true;
       }
+      if (CurrentToken->is(tok::greatergreatergreater)) {
+        // Treat >>> as 3 tokens in this context
+        static int Count;
+        if (Count == 0) Count = 3;
+
+        Left->MatchingParen = CurrentToken;
+        CurrentToken->MatchingParen = Left;
+        CurrentToken->Type = TT_TemplateCloser;
+        --Count;
+
+        // Only increment parser if 3 calls have been made
+        if (Count == 0) next();
+        return true;
+      }
       if (CurrentToken->is(tok::question) &&
           Style.Language == FormatStyle::LK_Java) {
         next();

              Left.isOneOf(TT_TemplateCloser, TT_TemplateOpener));
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
     return Style.SpacesInAngles;
+  // Treat parameters in between <<< and >>> similar to template parameters
+  if (Left.is(tok::lesslessless) && Right.isNot(tok::greatergreatergreater))
+    return Style.SpacesInAngles;
+  if (Right.is(tok::greatergreatergreater))
+    return Style.SpacesInAngles;
+  // No space needed between identifier or ">" and "<<<".
+  if ((Left.is(tok::identifier) || Left.is(tok::greater)) &&
+      Right.is(tok::lesslessless))
+    return false;
+  // No space needed between ">>>" and "("
+  if (Left.is(tok::greatergreatergreater) && Right.is(tok::l_paren))
+    return false;
   if ((Right.is(TT_BinaryOperator) && !Left.is(tok::l_paren)) ||
       Left.isOneOf(TT_BinaryOperator, TT_ConditionalExpr))
     return true;

unittests/Format/FormatTest.cpp

   verifyFormat("A<A<int>>();", Spaces);
 }
 
+TEST_F(FormatTest, TrippleAngleBraces) {
+  verifyFormat("f<<<1, 1>>>();");
+  verifyFormat("f<<<1, 1, 1, s>>>();");
+  verifyFormat("f<<<a, b, c, d>>>();");
+  EXPECT_EQ("f<<<1, 1>>>();",
+            format("f <<< 1, 1 >>> ();"));
+  verifyFormat("f<param><<<1, 1>>>();");
+  verifyFormat("f<1><<<1, 1>>>();");
+  EXPECT_EQ("f<param><<<1, 1>>>();",
+            format("f< param > <<< 1, 1 >>> ();"));
+}
+
 TEST_F(FormatTest, HandleUnbalancedImplicitBracesAcrossPPBranches) {
   std::string code = "#if A\n"
                      "#if B\n"