Page MenuHomePhabricator

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

Authored by chh on Jul 21 2017, 3:47 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

yawanng created this revision.Jul 21 2017, 3:47 PM
djasper edited edge metadata.Jul 31 2017, 10:03 PM

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 updated this revision to Diff 109222.Aug 1 2017, 2:15 PM

Move tests to unitest and fix a bug.

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 updated this revision to Diff 110640.Aug 10 2017, 3:24 PM

Show full context.

chh added a comment.Aug 14 2017, 12:04 PM

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.

klimek edited edge metadata.Sep 18 2017, 8:12 AM
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.

chh commandeered this revision.Sep 18 2017, 3:53 PM
chh added a reviewer: yawanng.
chh updated this revision to Diff 115741.Sep 18 2017, 3:56 PM
chh retitled this revision from [clang-format] Handle Structured binding declaration in C++17 to [clang-format] Adjust space around &/&& of structured bindings.
chh edited the summary of this revision. (Show Details)

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

curdeius edited edge metadata.Sep 19 2017, 2:48 AM

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();

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.

  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!)

chh updated this revision to Diff 116092.Sep 20 2017, 3:23 PM
chh edited the summary of this revision. (Show Details)

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?

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

chh updated this revision to Diff 116201.Sep 21 2017, 9:11 AM
chh marked 2 inline comments as done.
chh edited the summary of this revision. (Show Details)
chh edited the summary of this revision. (Show Details)
curdeius accepted this revision.Sep 26 2017, 1:28 AM

Thanks for the changes. LGTM.

This revision is now accepted and ready to land.Sep 26 2017, 1:28 AM
This revision was automatically updated to reflect the committed changes.