[clang-format] Handle object instansiation in if-statements

[thieta]

Summary

Before this patch code like this:

if (Class* obj{getObject()}) { }

would be mis-formated since the * would be annotated as a
binaryoperator.

This patch changes the * to become a PointerOrReference instead
and fixes the formatting issues.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

366

Could you add a test for this as well?

clang/unittests/Format/TokenAnnotatorTest.cpp

1050

There is already a test case for star amp understanding, please use that.

[thieta]

Expand and move testing

[thieta]

I have expanded testing and moved it to the place you suggested.

[HazardyKnusperkeks]

Thanks.

[MyDeveloperDay]

because of https://github.com/llvm/llvm-project/issues/61785 should this really be reverted? is basically saying X * Y { must be X *Y{ but that obviously not the case

[thieta]

In D137327#4233290, @MyDeveloperDay wrote:

because of https://github.com/llvm/llvm-project/issues/61785 should this really be reverted? is basically saying X * Y { must be X *Y{ but that obviously not the case

Tricky one. Any ideas on how we could differentiate those two cases? Maybe impossible? Not sure what the normal way to handle ambiguous things like that in clang-format is.

[MyDeveloperDay]

In D137327#4233551, @thieta wrote:

In D137327#4233290, @MyDeveloperDay wrote:

because of https://github.com/llvm/llvm-project/issues/61785 should this really be reverted? is basically saying X * Y { must be X *Y{ but that obviously not the case

Tricky one. Any ideas on how we could differentiate those two cases? Maybe impossible? Not sure what the normal way to handle ambiguous things like that in clang-format is.

I would prefer we avoid the regression that this issue caused, even if both are equally viable. because otherwise we get blamed for "changing defaults" @owenpan, @HazardyKnusperkeks what are your thoughts?

[HazardyKnusperkeks]

In D137327#4233652, @MyDeveloperDay wrote:

In D137327#4233551, @thieta wrote:

In D137327#4233290, @MyDeveloperDay wrote:

because of https://github.com/llvm/llvm-project/issues/61785 should this really be reverted? is basically saying X * Y { must be X *Y{ but that obviously not the case

Tricky one. Any ideas on how we could differentiate those two cases? Maybe impossible? Not sure what the normal way to handle ambiguous things like that in clang-format is.

I would prefer we avoid the regression that this issue caused, even if both are equally viable. because otherwise we get blamed for "changing defaults" @owenpan, @HazardyKnusperkeks what are your thoughts?

I have no idea when the release was/is. If it is not released it's a no brainer, revert. Otherwise I'm torn... for everyone skipping this release a revert would change nothing, for those who keep up to date we could be ping-ponging.

Can we solve the problem, without knowing what identifiers are types and what are objects/variables for all valid and not pathologic cases?

[thieta]

This was released in LLVM 16.0.0.

[owenpan]

In D137327#4234463, @thieta wrote:

This was released in LLVM 16.0.0.

I think we should revert it in 16.0.1.

[owenpan]

In D137327#4233551, @thieta wrote:

In D137327#4233290, @MyDeveloperDay wrote:

because of https://github.com/llvm/llvm-project/issues/61785 should this really be reverted? is basically saying X * Y { must be X *Y{ but that obviously not the case

Tricky one. Any ideas on how we could differentiate those two cases? Maybe impossible? Not sure what the normal way to handle ambiguous things like that in clang-format is.

We can add a Types option similar to StatementMacros.

[MyDeveloperDay]

I like both @owenpan suggestions

[MyDeveloperDay]

In D137327#4234463, @thieta wrote:

This was released in LLVM 16.0.0.

The prior behaviour was there before, it’s marked in GitHub as a regression, can you please revert, we’ll mark the issue to be cherry picked, then let’s go back and rework a solution that means your issue can be resolved

[thieta]

In D137327#4235255, @MyDeveloperDay wrote:

In D137327#4234463, @thieta wrote:

This was released in LLVM 16.0.0.

The prior behaviour was there before, it’s marked in GitHub as a regression, can you please revert, we’ll mark the issue to be cherry picked, then let’s go back and rework a solution that means your issue can be resolved

Sorry this slipped under my radar. Can you please push a revert to main and then file a issue to backport this to the release branch if it's something you still want to do. It's hard for me to keep track of the issues unless they are added to the 16.x milestone.

[MyDeveloperDay]

In D137327#4267239, @thieta wrote:

In D137327#4235255, @MyDeveloperDay wrote:

In D137327#4234463, @thieta wrote:

This was released in LLVM 16.0.0.

The prior behaviour was there before, it’s marked in GitHub as a regression, can you please revert, we’ll mark the issue to be cherry picked, then let’s go back and rework a solution that means your issue can be resolved

Sorry this slipped under my radar. Can you please push a revert to main and then file a issue to backport this to the release branch if it's something you still want to do. It's hard for me to keep track of the issues unless they are added to the 16.x milestone.

You have commit rights correct? you really need to own your change especially if it causes a regression.

[thieta]

In D137327#4267342, @MyDeveloperDay wrote:

You have commit rights correct? you really need to own your change especially if it causes a regression.

Alright - I did that now. Sorry I am just used to be on the other side as my role as a release manager and I thought you where asking me to revert it in that role.

I will see if I find time to add the new Types directive soon.

clang/lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
       // categorize it as an unary operator, so set the right type here.
       if (LookForDecls && CurrentToken->Next) {
         FormatToken *Prev = CurrentToken->getPreviousNonComment();
         if (Prev) {
           FormatToken *PrevPrev = Prev->getPreviousNonComment();
           FormatToken *Next = CurrentToken->Next;
           if (PrevPrev && PrevPrev->is(tok::identifier) &&
               Prev->isOneOf(tok::star, tok::amp, tok::ampamp) &&
-              CurrentToken->is(tok::identifier) && Next->isNot(tok::equal)) {
+              CurrentToken->is(tok::identifier) &&
+              !Next->isOneOf(tok::equal, tok::l_brace)) {
             Prev->setType(TT_BinaryOperator);
             LookForDecls = false;
           }
         }
       }
 
       if (CurrentToken->Previous->is(TT_PointerOrReference) &&
           CurrentToken->Previous->Previous->isOneOf(tok::l_paren,
TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
     // the `}` are put on separate unwrapped lines; in the brace-initialized
     // case, the matching `{` is on the same unwrapped line, so check for the
     // presence of the matching brace to distinguish between those.
     if (PrevToken->is(tok::r_brace) && Tok.is(tok::star) &&
         !PrevToken->MatchingParen) {
       return TT_PointerOrReference;
     }
 
+    // if (Class* obj { function() })
+    if (PrevToken->Tok.isAnyIdentifier() && NextToken->Tok.isAnyIdentifier() &&
+        NextToken->Next && NextToken->Next->is(tok::l_brace)) {
+      return TT_PointerOrReference;
+    }
+
     if (PrevToken->endsSequence(tok::r_square, tok::l_square, tok::kw_delete))
       return TT_UnaryOperator;
 
     if (PrevToken->Tok.isLiteral() ||
         PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
                            tok::kw_false, tok::r_brace)) {
       return TT_BinaryOperator;
     }

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmp) {
   EXPECT_TOKEN(Tokens[7], tok::star, TT_UnaryOperator);
   EXPECT_TOKEN(Tokens[12], tok::star, TT_PointerOrReference);
 
   Tokens = annotate("void f() { void (*fnptr)(t* foo); }");
   EXPECT_EQ(Tokens.size(), 18u) << Tokens;
   EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_FunctionTypeLParen);
   EXPECT_TOKEN(Tokens[7], tok::star, TT_UnaryOperator);
   EXPECT_TOKEN(Tokens[12], tok::star, TT_PointerOrReference);
+
+  Tokens = annotate("if (Foo * Bar / Test)");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::star, TT_BinaryOperator);
+
+  Tokens = annotate("if (Class* obj {getObj()})");
+  ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::star, TT_PointerOrReference);
+
+  Tokens = annotate("if (Foo* Bar = getObj())");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::star, TT_PointerOrReference);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
   auto Tokens = annotate("x - 0");
   ASSERT_EQ(Tokens.size(), 4u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::minus, TT_BinaryOperator);
   Tokens = annotate("0 + 0");
   ASSERT_EQ(Tokens.size(), 4u) << Tokens;
TEST_F(TokenAnnotatorTest, UnderstandsFunctionAnnotations) {
   EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_FunctionAnnotationRParen);
 
   Tokens = annotate("template <typename T>\n"
                     "A(T) noexcept;");
   ASSERT_EQ(Tokens.size(), 12u) << Tokens;
   EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_Unknown);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsVerilogOperators) {
   auto Annotate = [this](llvm::StringRef Code) {
     return annotate(Code, getLLVMStyle(FormatStyle::LK_Verilog));
   };
   // Test that unary operators get labeled as such and that operators like '++'
   // don't get split.
   tok::TokenKind Unary[] = {tok::plus,  tok::minus,    tok::exclaim,
                             tok::tilde, tok::amp,      tok::pipe,
                             tok::caret, tok::plusplus, tok::minusminus};