[clang-format] Put peekNextToken(/*SkipComment=*/true) to good use

[owenpan]

Summary

To prevent potential bugs in situations where we want to peek the next non-comment token.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

We should pass true to peekNextToken() on Line 3597 here, but removing this entire case would have no effect on the existing unit test (Line 23693 in FormatTest.cpp). @HazardyKnusperkeks any idea?

[rymiel]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

The peeking is there because of https://reviews.llvm.org/D134325, it has no format tests, only an annotator test

[rymiel]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

Also, I don't think that function is involved in concept declarations since https://reviews.llvm.org/D140339

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

So which tests were meant to check the kw_bool case here?

[rymiel]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

Well, originally the one you mentioned on line 23693, but since the code paths have been changed, I think it's really only the one added in https://reviews.llvm.org/D134325

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

I love the new annotator tests, but we should probably always try to add a verifyFormat test as well even if its trivial in the future, just so we have some context as to what type of code warranted the change.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

As far as I can see it does not only can be removed, it should be removed.

bool can not appear on the top level of a requires clause. As @rymiel said it is there for the concepts, which are now handled differently.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

@rymiel @HazardyKnusperkeks thanks for the explanations. Can we remove the kw_bool case then?

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

As far as I can see it does not only can be removed, it should be removed.

Okay, I will leave it to you or @rymiel then. :)

[rymiel]

clang/lib/Format/UnwrappedLineParser.cpp

3592–3601

As far as I can see it does not only can be removed, it should be removed.

You are right! I forgot the fact that this only ever applied for concepts, and the bug fix only ever worked around that. I had it backwards in my head because the bug affected requires clauses too.

clang/lib/Format/UnwrappedLineParser.cpp

case tok::r_brace:
       addUnwrappedLine();
       return;
     case tok::l_paren: {
       parseParens();
       // Break the unwrapped line if a K&R C function definition has a parameter
       // declaration.
       if (!IsTopLevel || !Style.isCpp() || !Previous || eof())
         break;
-      if (isC78ParameterDecl(FormatTok, Tokens->peekNextToken(), Previous)) {
+      if (isC78ParameterDecl(FormatTok,
+                             Tokens->peekNextToken(/*SkipComment=*/true),
+                             Previous)) {
         addUnwrappedLine();
         return;
       }
       break;
     }
     case tok::kw_operator:
       nextToken();
       if (FormatTok->isBinaryOperator())
if (Previous &&
                          tok::kw_delete, tok::l_square) ||
        LeftSquare->isCppStructuredBinding(Style) || Previous->closesScope() ||
        Previous->isSimpleTypeSpecifier())) {
     return false;
   }
   if (FormatTok->is(tok::l_square))
     return false;
   if (FormatTok->is(tok::r_square)) {
-    const FormatToken *Next = Tokens->peekNextToken();
+    const FormatToken *Next = Tokens->peekNextToken(/*SkipComment=*/true);
     if (Next->is(tok::greater))
       return false;
   }
   parseSquare(/*LambdaIntroducer=*/true);
   return true;
 }
 
 void UnwrappedLineParser::tryToParseJSFunction() {
case tok::kw_dynamic_cast:
       if (!FormatTok->is(tok::less))
         return;
 
       nextToken();
       parseBracedList(/*ContinueOnSemicolons=*/false, /*IsEnum=*/false,
                       /*ClosingBraceKind=*/tok::greater);
       break;
 
     case tok::kw_bool:
       // bool is only allowed if it is directly followed by a paren for a cast:
       // concept C = bool(...);
       // and bool is the only type, all other types as cast must be inside a
       // cast to bool an thus are handled by the other cases.
       if (Tokens->peekNextToken()->isNot(tok::l_paren))
         return;
       nextToken();
       parseParens();
       break;
 
     default:
       if (!FormatTok->Tok.getIdentifierInfo()) {
         // Identifiers are part of the default case, we check for more then
         // tok::identifier to handle builtin type traits.
         return;
       }
 

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsAccessSpecifiers) {
   verifyFormat("public::foo();");
 
   verifyFormat("class A {\n"
                "public:\n"
                "  std::unique_ptr<int *[]> b() { return nullptr; }\n"
                "\n"
                "private:\n"
                "  int c;\n"
+               "};\n"
+               "class B {\n"
+               "public:\n"
+               "  std::unique_ptr<int *[] /* okay */> b() { return nullptr; }\n"
+               "\n"
+               "private:\n"
+               "  int c;\n"
                "};");
 }
 
 TEST_F(FormatTest, SeparatesLogicalBlocks) {
   EXPECT_EQ("class A {\n"
             "public:\n"
             "  void f();\n"
             "\n"
verifyFormat("int f(a, b, c);\n" // No break here.
                Style);
   verifyFormat("byte *\n" // Break here.
                "f(a)\n"   // Break here.
                "byte a[];\n"
                "{\n"
                "  return a;\n"
                "}",
                Style);
+  verifyFormat("byte *\n"
+               "f(a)\n"
+               "byte /* K&R C */ a[];\n"
+               "{\n"
+               "  return a;\n"
+               "}\n"
+               "byte *\n"
+               "g(p)\n"
+               "byte /* K&R C */ *p;\n"
+               "{\n"
+               "  return p;\n"
+               "}",
+               Style);
   verifyFormat("bool f(int a, int) override;\n"
                "Bar g(int a, Bar) final;\n"
                "Bar h(a, Bar) final;",
                Style);
   verifyFormat("int\n"
                "f(a)",
                Style);
   verifyFormat("bool\n"