[clang-format] Fix inconsistent identification of operator&

[dkt01]

Summary

Token annotator for clang-format incorrectly identifies operator& as a reference type in situations like Boost serialization archives.

Add annotation rules for standalone and chained operator& instances while preserving behavior for reference declarations at class scope. Add tests to validate annotation and formatting behavior.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

2474–2475

This case was already handled properly by the IsExpression && !Contexts.back().CaretFound condition, but I'll add this test case as there isn't an equivalent test yet.

[dkt01]

Changes based on HazardyKnusperkeks' review

[dkt01]

Further changes based on HazardyKnusperkeks' review

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1200

I previously had a double pop for some braces, but now the checks are no longer necessary as pushes and pops match.

[MyDeveloperDay]

clang/unittests/Format/TokenAnnotatorTest.cpp

181

how does this differentiate from

MyType & val2;

is that a binary operator? I don't think so?

[dkt01]

clang/unittests/Format/TokenAnnotatorTest.cpp

181

The heuristic I'm using is that [symbol] & [symbol]; outside a class/struct declaration is more likely an instance of operator& than an uninitialized reference. Tests on line 206 and 215 demonstrate how the two cases differ.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2525

The TokenAnnotator? And then by reference to the parser.

2926

Unrelated.

clang/unittests/Format/TokenAnnotatorTest.cpp

181

The heuristic I'm using is that [symbol] & [symbol]; outside a class/struct declaration is more likely an instance of operator& than an uninitialized reference. Tests on line 206 and 215 demonstrate how the two cases differ.

I'm with this one. A reference without initialization is an error.

Can you add a test with an assignment, I didn't see one right now.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

2525

I was over-thinking it... Adding to TokenAnnotator works great!
I did have to re-introduce the size checks before popping scopes because otherwise the UnbalancedStructuralElements test fails.

clang/unittests/Format/TokenAnnotatorTest.cpp

181

Added two new tests for assignment to pointer using unary operator & and initializing assignment to reference because neither of these assignments was in existing format tests I could find.

[dkt01]

Changes as suggested by HazardyKnusperkeks in review.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

867

See below

868

Is this check also necessary? Here we should only be when we have pushed before.
I'd change the if for an assert.

1200

Move the comment in the if? In general it hasn't to be a unbalanced brace, right?

[dkt01]

Changes recommended by HazardyKnusperkeks in review.

[HazardyKnusperkeks]

Looks good to me, but please wait until another one has given their blessing, or at least no veto for a few days.

[MyDeveloperDay]

There is part of me that feels we should not rush this in to get it into 16 (which was due to branch today), I feel it might be a good idea to commit this after branching, and let it live in 'main' so we can take for a real test drive

[dkt01]

Sounds good. I don't believe I have permissions to add commits to three repo anyway, so someone can add on my behalf after the 16 branch.

[MyDeveloperDay]

I think they have branched (https://clang.llvm.org/docs/ClangFormatStyleOptions.html) the documentation has changed to 17.0

We would need your name and email in order to commit on your behalf.

I checked this patch out and tested it on my large project and I didn't see anything immediately jump out.

@owenpan, @rymiel any concerns?

[dkt01]

David Turner <[e-mail]>

Good to know it worked as expected on your project as well.

[MyDeveloperDay]

Good to know it worked as expected on your project as well.

I have some concerns (mainly because it feels like it could be quite invasive), hence I'm delaying on the LGTM, I really want to run this on a large code base, which I think if we commit post the branch (which is now branched) we would probably see more easily. Historically in the past I think the firefox team have kindly picked up recent builds (no obligations) and catch any regressions.

clang/lib/Format/TokenAnnotator.cpp

2768

was there a reason it could no longer be const?

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

119–120

Please keep them in the same order as that of the parameters above.

868

Should we assert that the token type of OpeningBrace matches the ScopeType before popping?

1179–1186

Do we really need these? If we add other special l_brace token types, we will have to update this list.

1203

Can you leave the comment at the top of the case? If we get rid of None for ScopeType, we don't need to pop here. (See below.)

2481–2483

Can you add a lambda for this so that it can be used below?

2484–2486

NextToken is guarded against null on line 2488 but not on lines 2489-2490.

2487–2488

Here we can use the lambda suggested above.

2778

Instead of pushing None, would it better to leave Scopes empty? Then we would not need None for the ScopeType, and instead of checking for Scopes.size() > 1, we could check if it's empty().

clang/lib/Format/TokenAnnotator.h

193

Drop std::.

clang/unittests/Format/TokenAnnotatorTest.cpp

192–194

Can you add a case for periodstar?

207–213

Can you add a case for arrowstar?

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1179–1186

Agreed. This was left over from a prior implementation that didn't have a default case.

2484–2486

If NextToken is null on 2488, 2489-2490 will be short circuited.

2768

Member Scopes can be modified. Alternate would be to make Scopes mutable, but I thought removing const from this function is cleaner

[dkt01]

Changes as suggested in review from owenpan

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

115–117

Something like the above. (I prefer Scopes to TrackedScopes to be consistent with the naming of the other parameters.)

1200–1203

I don't think it's about unbalanced braces here.

2479–2480

To help simplify the if statement below.

2484–2486

You are right. The redundant parens threw me off.

2485

Redundant parens.

2486–2487

The lambda would check that PrevToken is nonnull.

2488–2492

Something like that.

clang/lib/Format/TokenAnnotator.h

36–45

Consider moving ScopeType out of TokenAnnotator and keeping it consistent with the style of LineType above (and replacing TokenAnnotator::ScopeType with ScopeType everywhere).

193–200

See above.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1200–1203

if (!Scopes.empty()) handles unbalanced braces. if(Tok->Previous) handles the case where a line starts with an rbrace.

2486–2487

!PrevToken->getPreviousNonComment() || is a different check than the null check in the lambda. It's acceptable to have no token two tokens prior because that indicates the ampersand is the second token of the line.

[dkt01]

Updates suggested in owenpan's review.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

123–125

129

868

1169–1181

1200–1203

I can't think of an example. Do you have one?

2479

Sorry about that!

2486–2487

I actually wanted fold this into the lambda. (See above.)

2486–2487

Now we only need the lambda.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1200–1203

The format unit test FormatUnbalancedStructuralElements feeds in strings that have only right braces, so without this check the pop fails.

[dkt01]

Updates recommended by owenpan in review.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

123

As suggested before.

1200–1203

Thanks!

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

123

Oops... missed the consts last time...

[dkt01]

Changes requested in owenpan's review.
Rebased changes onto latest main branch.