[PATCH] D121754: [clang-format] Refactor determineStarAmpUsage

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

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2146-2147
+    //   know how they can be followed by a star or amp.
+    // co_await, delete - It doesn't make sense to have them followed by a unary
+    //   `+` or `-`.
+    if (PrevToken->isOneOf(TT_ConditionalExpr, tok::l_paren, tok::comma,
----------------
sstwcw wrote:
> sstwcw wrote:
> > MyDeveloperDay wrote:
> > > HazardyKnusperkeks wrote:
> > > > Especially here, why should a `+` after `delete` be a binary operator?
> > > > How much sense it makes doesn't matter, it is valid c++: https://gcc.godbolt.org/z/c1x1nn3Ej
> > > I'm also trying to understand did you mean you couldn't have
> > > 
> > > case -1:
> > > case +1:
> > > case +0:
> > > case  -0:
> > > 
> > > https://gcc.godbolt.org/z/qvE44d5xz
> > In the new version `+` following `delete` is a unary operator.  Previously I was under the impression that we only formatted code that is sensible.
> > case -1:
> > case +1:
> 
> I meant we couldn't have things like `case *x` or `case &x`.  Is the comment not clear enough?
> > case -1:
> > case +1:
> 
> I meant we couldn't have things like `case *x` or `case &x`.  Is the comment not clear enough?

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

> In the new version `+` following `delete` is a unary operator.  Previously I was under the impression that we only formatted code that is sensible.

What is `sensible` this is rather subjective, isn't it? I went a long way to get requires clauses and expressions doing to "right thing" and even added explicitly the unit tests for stuff I think no sane person would ever write, but it's valid code. So in my opinion it should be formatted correctly. I got it wrong on a couple of cases, all but one that I'm aware of are fixed now.

And even if it were invalid code, when it makes the functions to reason about easier why not? **I** think invalid code can be formatted weirdly, once it's valid again we will format it correctly. But all valid code should be formatted as best as we can.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2146-2147
+    //   know how they can be followed by a star or amp.
+    // co_await, delete - It doesn't make sense to have them followed by a unary
+    //   `+` or `-`.
+    if (PrevToken->isOneOf(TT_ConditionalExpr, tok::l_paren, tok::comma,
----------------
HazardyKnusperkeks wrote:
> sstwcw wrote:
> > sstwcw wrote:
> > > MyDeveloperDay wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > Especially here, why should a `+` after `delete` be a binary operator?
> > > > > How much sense it makes doesn't matter, it is valid c++: https://gcc.godbolt.org/z/c1x1nn3Ej
> > > > I'm also trying to understand did you mean you couldn't have
> > > > 
> > > > case -1:
> > > > case +1:
> > > > case +0:
> > > > case  -0:
> > > > 
> > > > https://gcc.godbolt.org/z/qvE44d5xz
> > > In the new version `+` following `delete` is a unary operator.  Previously I was under the impression that we only formatted code that is sensible.
> > > case -1:
> > > case +1:
> > 
> > I meant we couldn't have things like `case *x` or `case &x`.  Is the comment not clear enough?
> > > case -1:
> > > case +1:
> > 
> > I meant we couldn't have things like `case *x` or `case &x`.  Is the comment not clear enough?
> 
> But we can: https://gcc.godbolt.org/z/8Mb1xo7oP
> 
> > In the new version `+` following `delete` is a unary operator.  Previously I was under the impression that we only formatted code that is sensible.
> 
> What is `sensible` this is rather subjective, isn't it? I went a long way to get requires clauses and expressions doing to "right thing" and even added explicitly the unit tests for stuff I think no sane person would ever write, but it's valid code. So in my opinion it should be formatted correctly. I got it wrong on a couple of cases, all but one that I'm aware of are fixed now.
> 
> And even if it were invalid code, when it makes the functions to reason about easier why not? **I** think invalid code can be formatted weirdly, once it's valid again we will format it correctly. But all valid code should be formatted as best as we can.
> In the new version `+` following `delete` is a unary operator.  Previously I was under the impression that we only formatted code that is sensible.




================
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:85
+  auto Tokens = annotate("x - 0");
+  EXPECT_EQ(Tokens.size(), 4u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::minus, TT_BinaryOperator);
----------------
I know the other cases also use EXPECT, but the size should be an ASSERT, so there would be no seg fault if the number would change (it shouldn't).


================
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:157
+  EXPECT_TOKEN(Tokens[1], tok::plus, TT_UnaryOperator);
+  Tokens = annotate("(int)-x");
+  EXPECT_EQ(Tokens.size(), 6u) << Tokens;
----------------
This is the Cast Paren case, right? Please also add the paren check.


================
Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:162
+  EXPECT_EQ(Tokens.size(), 4u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::plus, TT_UnaryOperator);
+}
----------------
The exclaim too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121754