[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:93
+  EXPECT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::star, TT_PointerOrReference);
 }

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > Can you add a verifyFormat test that shows what you want? as well
> I think the annotator test is sufficient. Because it's just about annotating 
> the token, formatting is secondary (and dependent on style - these tests are 
> already in place).
I know where you are coming from, but actually if it wasn't for the 
`enable_if<>{} && ...` example in the unit tests then we'd have missed the && 
case and caused a regression. (that was ONLY covered by the small example code 
snippet)

Whilst I believe the TokenAnnotator tests are correct to have as well, I think 
we should be adding formatting examples just to ensure someone doesn't breaking 
the formatting rule that this depends on.  I always follow the Beyoncé rule... 
"If you like it you should have put a test on it!"

So If you don't want anyone to break your `*` placement after the `}` then I 
see no harm in adding a single verifyFormat(), but while you are at it please 
also test to ensure the Left/Middle/Right works with your example as you might 
expect. (just incase there are some rules about that, that could interfere with 
your setting)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127873/new/

https://reviews.llvm.org/D127873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

Could you please either directly link to the github issue or better use 
llvm.org/pr55810 than just #55810 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127873/new/

https://reviews.llvm.org/D127873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D127873#3586475 , @MyDeveloperDay 
wrote:

> what about (may not be useful but compiles)
>
>   struct {
>   int foo;
>   } &&ptr2 = {};
>
> https://godbolt.org/z/rbb8x3hKP

Let's ask why it was set to a binary operator. What kind of code would want the 
binary operator after a closing brace? I can make up such code, but we 
basically have to decide which code is probably more in use, or make the 
detection more sophisticated.




Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:93
+  EXPECT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::star, TT_PointerOrReference);
 }

MyDeveloperDay wrote:
> Can you add a verifyFormat test that shows what you want? as well
I think the annotator test is sufficient. Because it's just about annotating 
the token, formatting is secondary (and dependent on style - these tests are 
already in place).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127873/new/

https://reviews.llvm.org/D127873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.

Thank you for the patch, just some observations




Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:93
+  EXPECT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::star, TT_PointerOrReference);
 }

Can you add a verifyFormat test that shows what you want? as well


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127873/new/

https://reviews.llvm.org/D127873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

what about (may not be useful but compiles)

  struct {
  int foo;
  } &&ptr2 = {};

https://godbolt.org/z/rbb8x3hKP


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127873/new/

https://reviews.llvm.org/D127873

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127873: [clang-format] Fix misplacemnt of `*` in declartion of pointer to struct

[Jack Huang via Phabricator via cfe-commits]

jackhong12 created this revision.
jackhong12 added a reviewer: clang-format.
Herald added a project: All.
jackhong12 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes #55810

Star tokens should be categorized as TT_PointerOrReference instead of 
TT_BinaryOperator when behind the right bracket.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127873

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp


Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -85,6 +85,12 @@
   Tokens = annotate("case &x:");
   EXPECT_EQ(Tokens.size(), 5u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::amp, TT_UnaryOperator);
+
+  Tokens = annotate("struct {\n"
+"  int foo;\n"
+"} *ptr;\n");
+  EXPECT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::star, TT_PointerOrReference);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2306,10 +2306,13 @@
 
 if (PrevToken->Tok.isLiteral() ||
 PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
-   tok::kw_false, tok::r_brace)) {
+   tok::kw_false)) {
   return TT_BinaryOperator;
 }
 
+if (PrevToken->is(tok::r_brace) && Tok.isOneOf(tok::amp, tok::ampamp))
+  return TT_BinaryOperator;
+
 const FormatToken *NextNonParen = NextToken;
 while (NextNonParen && NextNonParen->is(tok::l_paren))
   NextNonParen = NextNonParen->getNextNonComment();


Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -85,6 +85,12 @@
   Tokens = annotate("case &x:");
   EXPECT_EQ(Tokens.size(), 5u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::amp, TT_UnaryOperator);
+
+  Tokens = annotate("struct {\n"
+"  int foo;\n"
+"} *ptr;\n");
+  EXPECT_EQ(Tokens.size(), 10u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::star, TT_PointerOrReference);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2306,10 +2306,13 @@
 
 if (PrevToken->Tok.isLiteral() ||
 PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
-   tok::kw_false, tok::r_brace)) {
+   tok::kw_false)) {
   return TT_BinaryOperator;
 }
 
+if (PrevToken->is(tok::r_brace) && Tok.isOneOf(tok::amp, tok::ampamp))
+  return TT_BinaryOperator;
+
 const FormatToken *NextNonParen = NextToken;
 while (NextNonParen && NextNonParen->is(tok::l_paren))
   NextNonParen = NextNonParen->getNextNonComment();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits