[clang-format] Adjust space around &/&& of structured bindings

[chh]

Summary

Keep space before or after the &/&& tokens, but not both. For example,

auto [x,y] = a; 
auto &[xr, yr] = a; // LLVM style
auto& [xr, yr] = a; // google style

[yawanng]

Ping.

[djasper]

Please add all the tests into unittests/Format/FormatTest.cpp instead. We use FileCheck-based tests only to verify the behavior of the clang-format binary itself.

[yawanng]

Move tests to unitest and fix a bug.

[djasper]

Generally, please upload patches with full context to phabricator. (or use arc)

I think this approach is a bit inside out. We are in a codepath where we try to find a lambda introducer and we the look one or two tokens back to see that we aren't as we have seen "auto". I wonder whether we could instead see "auto" and understand that now we have to parse a declaration. Manuel, any thoughts?

[yawanng]

Show full context.

[chh]

Daniel, Manuel, I will take over this CL since Yan has finished his internship at Google.,
Yan's latest patch to tryToParseLambda looks acceptable to me.
I think it should take care of new kw_auto in additional to kw_new, ke_delete, etc.

Could you suggest if there is any better way to handle the new syntax?
Thanks.

[chh]

ping.

[klimek]

In D35743#841197, @chh wrote:

Daniel, Manuel, I will take over this CL since Yan has finished his internship at Google.,
Yan's latest patch to tryToParseLambda looks acceptable to me.
I think it should take care of new kw_auto in additional to kw_new, ke_delete, etc.

Could you suggest if there is any better way to handle the new syntax?

An alternative would be to look for auto (&&?)? [ with look-ahead, but I agree that this fits the "we try to parse a lambda introducer with look-behind" strategy we've so far been taking, so I'm fine with this approach.

[klimek]

So we actually didn't need to change the UnwrappedLineParser? I assume we still incorrectly assume the structured bindings are labmdas then?

[curdeius]

There is one big missing thing here: PointerAlignment. Actually only PAS_Left is taken into account.
There are 3 possible options:
Left: auto& [a, b] = f();
Middle: auto & [a, b] = f();
Right: auto &[a, b] = f();

[klimek]

Ok, we still need to fix structured bindings in the UnwrappedLineParser. Unfortunately isCppStructuredBinding requires a "previous token" function, which we don't really have in the UnwrappedLineParser. /me goes thinking more about that part of the problem. That should be largely independent of this patch, though, so LG from my side.

[klimek]

1. Can you rebase after r313742? I fixed detection of structured bindings in the UnwrappedLineParser, that might affect this patch
2. Isn't LLVM style the reverse? That is, shouldn't that be
   
   LLVM style:
   
   auto &&[a, b] = ...
   
   Pointer/Ref-to-type-style:
   
   auto&& [a, b] = ...

(and I just notice that I didn't fully clang-format my last patch, argh!)

[klimek]

Apart from nits, LGTM, unless Daniel sees issues.

lib/Format/TokenAnnotator.cpp

2516 ↗

(On Diff #116092)

Super-Nit: can you start with a capital letter and end with a '.'?

unittests/Format/FormatTest.cpp

11604–11610 ↗

(On Diff #116092)

I don't think the non-structured-binding tests are necessary here, as they're testing something completely different?

[curdeius]

Could you add just one more test for PAS_Middle please? A single line will be just enough.
Otherwise, LGTM.

[chh]

ping.

[curdeius]

Thanks for the changes. LGTM.

cfe/trunk/lib/Format/TokenAnnotator.cpp

bool StartsObjCMethodExpr =
          Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
                          tok::kw_return, tok::kw_throw) ||
          Parent->isUnaryOperator() ||
          Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
          getBinOpPrecedence(Parent->Tok.getKind(), true, true) > prec::Unknown);
     bool ColonFound = false;
 
     unsigned BindingIncrease = 1;
-    if (Left->is(TT_Unknown)) {
-      if (Left->isCppStructuredBinding(Style)) {
-        Left->Type = TT_StructuredBindingLSquare;
-      } else if (StartsObjCMethodExpr) {
+    } else if (Left->is(TT_Unknown)) {
+      if (StartsObjCMethodExpr) {
         Left->Type = TT_ObjCMethodExpr;
       } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
                  Contexts.back().ContextKind == tok::l_brace &&
                  Parent->isOneOf(tok::l_brace, tok::comma)) {
         Left->Type = TT_JsComputedPropertyName;
       } else if (Style.isCpp() && Contexts.back().ContextKind == tok::l_brace &&
                  Parent && Parent->isOneOf(tok::l_brace, tok::comma)) {
         Left->Type = TT_DesignatedInitializerLSquare;
bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
   if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
     return (Left.is(TT_TemplateOpener) &&
             Style.Standard == FormatStyle::LS_Cpp03) ||
            !(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
                           tok::kw___super, TT_TemplateCloser,
                           TT_TemplateOpener));
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
     return Style.SpacesInAngles;
+  // Space before TT_StructuredBindingLSquare.
+  if (Right.is(TT_StructuredBindingLSquare))
+    return !Left.isOneOf(tok::amp, tok::ampamp) ||
+           Style.PointerAlignment != FormatStyle::PAS_Right;
+  // Space before & or && following a TT_StructuredBindingLSquare.
+  if (Right.Next && Right.Next->is(TT_StructuredBindingLSquare) &&
+      Right.isOneOf(tok::amp, tok::ampamp))
+    return Style.PointerAlignment != FormatStyle::PAS_Left;
   if ((Right.is(TT_BinaryOperator) && !Left.is(tok::l_paren)) ||
       (Left.isOneOf(TT_BinaryOperator, TT_ConditionalExpr) &&
        !Right.is(tok::r_paren)))
     return true;
   if (Left.is(TT_TemplateCloser) && Right.is(tok::l_paren) &&
       Right.isNot(TT_FunctionTypeLParen))
     return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, StructuredBindings) {
   // all modes, including C++11, C++14 and C++17
   verifyFormat("auto [a, b] = f();");
   EXPECT_EQ("auto [a, b] = f();", format("auto[a, b] = f();"));
   EXPECT_EQ("const auto [a, b] = f();", format("const   auto[a, b] = f();"));
   EXPECT_EQ("auto const [a, b] = f();", format("auto  const[a, b] = f();"));
   EXPECT_EQ("auto const volatile [a, b] = f();",
             format("auto  const   volatile[a, b] = f();"));
   EXPECT_EQ("auto [a, b, c] = f();", format("auto   [  a  ,  b,c   ] = f();"));
-  EXPECT_EQ("auto & [a, b, c] = f();",
+  EXPECT_EQ("auto &[a, b, c] = f();",
             format("auto   &[  a  ,  b,c   ] = f();"));
-  EXPECT_EQ("auto && [a, b, c] = f();",
+  EXPECT_EQ("auto &&[a, b, c] = f();",
             format("auto   &&[  a  ,  b,c   ] = f();"));
-  EXPECT_EQ("auto const & [a, b] = f();", format("auto  const&[a, b] = f();"));
-  EXPECT_EQ("auto const volatile && [a, b] = f();",
+  EXPECT_EQ("auto const &[a, b] = f();", format("auto  const&[a, b] = f();"));
+  EXPECT_EQ("auto const volatile &&[a, b] = f();",
             format("auto  const  volatile  &&[a, b] = f();"));
-  EXPECT_EQ("auto && [a, b] = f();", format("auto  &&[a, b] = f();"));
+  EXPECT_EQ("auto const &&[a, b] = f();", format("auto  const   &&  [a, b] = f();"));
+  EXPECT_EQ("const auto &[a, b] = f();", format("const  auto  &  [a, b] = f();"));
+  EXPECT_EQ("const auto volatile &&[a, b] = f();",
+            format("const  auto   volatile  &&[a, b] = f();"));
+  EXPECT_EQ("volatile const auto &&[a, b] = f();",
+            format("volatile  const  auto   &&[a, b] = f();"));
+  EXPECT_EQ("const auto &&[a, b] = f();", format("const  auto  &&  [a, b] = f();"));
 
   // Make sure we don't mistake structured bindings for lambdas.
-  verifyFormat("auto [a, b]{A * i};");
-  verifyFormat("auto const [a, b]{A * i};");
-  verifyFormat("auto const && [a, b]{A * i};");
+  FormatStyle PointerMiddle = getLLVMStyle();
+  PointerMiddle.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("auto [a1, b]{A * i};", getGoogleStyle());
+  verifyFormat("auto [a2, b]{A * i};", getLLVMStyle());
+  verifyFormat("auto [a3, b]{A * i};", PointerMiddle);
+  verifyFormat("auto const [a1, b]{A * i};", getGoogleStyle());
+  verifyFormat("auto const [a2, b]{A * i};", getLLVMStyle());
+  verifyFormat("auto const [a3, b]{A * i};", PointerMiddle);
+  verifyFormat("auto const& [a1, b]{A * i};", getGoogleStyle());
+  verifyFormat("auto const &[a2, b]{A * i};", getLLVMStyle());
+  verifyFormat("auto const & [a3, b]{A * i};", PointerMiddle);
+  verifyFormat("auto const&& [a1, b]{A * i};", getGoogleStyle());
+  verifyFormat("auto const &&[a2, b]{A * i};", getLLVMStyle());
+  verifyFormat("auto const && [a3, b]{A * i};", PointerMiddle);
+
+  EXPECT_EQ("for (const auto &&[a, b] : some_range) {\n}",
+            format("for (const auto   &&   [a, b] : some_range) {\n}"));
+  EXPECT_EQ("for (const auto &[a, b] : some_range) {\n}",
+            format("for (const auto   &   [a, b] : some_range) {\n}"));
+  EXPECT_EQ("for (const auto [a, b] : some_range) {\n}",
+            format("for (const auto[a, b] : some_range) {\n}"));
+  EXPECT_EQ("auto [x, y](expr);", format("auto[x,y]  (expr);"));
+  EXPECT_EQ("auto &[x, y](expr);", format("auto  &  [x,y]  (expr);"));
+  EXPECT_EQ("auto &&[x, y](expr);", format("auto  &&  [x,y]  (expr);"));
+  EXPECT_EQ("auto const &[x, y](expr);", format("auto  const  &  [x,y]  (expr);"));
+  EXPECT_EQ("auto const &&[x, y](expr);", format("auto  const  &&  [x,y]  (expr);"));
+  EXPECT_EQ("auto [x, y]{expr};", format("auto[x,y]     {expr};"));
+  EXPECT_EQ("auto const &[x, y]{expr};", format("auto  const  &  [x,y]  {expr};"));
+  EXPECT_EQ("auto const &&[x, y]{expr};", format("auto  const  &&  [x,y]  {expr};"));
 
   format::FormatStyle Spaces = format::getLLVMStyle();
   Spaces.SpacesInSquareBrackets = true;
   verifyFormat("auto [ a, b ] = f();", Spaces);
-  verifyFormat("auto && [ a, b ] = f();", Spaces);
+  verifyFormat("auto &&[ a, b ] = f();", Spaces);
+  verifyFormat("auto &[ a, b ] = f();", Spaces);
+  verifyFormat("auto const &&[ a, b ] = f();", Spaces);
+  verifyFormat("auto const &[ a, b ] = f();", Spaces);
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang