[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8f1156a7d004: [clang-format] Fix an ObjC regression 
introduced with new [[likely]]… (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestObjC.cpp

Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,25 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+  verifyFormat("if (argc < 5) [[obj1 method1:arg1] method2:arg2];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]][1];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -16513,6 +16513,11 @@
"  return 42;\n"
"}\n",
Style);
+
+  verifyFormat("if (argc > 5) [[gnu::unused]] {\n"
+   "  return 29;\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LLVMDefaultStyle) {
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseSimpleAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseSimpleAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,51 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+namespace {
+// A class used to set and restore the Token position when peeking
+// ahead in the token source.
+class ScopedTokenPosition {
+  unsigned StoredPosition;
+  FormatTokenSource *Tokens;
+
+public:
+  ScopedTokenPosition(FormatTokenSource *Tokens) : Tokens(Tokens) {
+assert(Tokens && "Tokens expected to not be null");
+StoredPosition = Tokens->getPosition();
+  }
+
+  ~ScopedTokenPosition() { Tokens->setPosition(StoredPosition); }
+};
+} // namespace
+
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseSimpleAttribute() {
+  ScopedTokenPosition AutoPosition(Tokens);
+  FormatToken *Tok = Tokens->getNextToken();
+  // We already read the first [ check for the second.
+  if (Tok && !Tok->is(tok::l_square)) {
+return false;
+  }
+  // Double check that the attribute is just something
+  // fairly simple.
+  while (Tok) {
+if (Tok->is(tok::r_square)) {
+  break;
+}
+Tok = Tokens->getNextToken();
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && !Tok->is(tok::r_square)) {
+return false;
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && Tok->is(tok::semi)) {
+return false;
+  }
+  return true;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

This is nice! Thank you!

Ran this through a mix of C++ and Objective-C sources and didn't spot any 
regressions from pre-D80144.


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266248.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Address nits


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

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestObjC.cpp

Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,25 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+  verifyFormat("if (argc < 5) [[obj1 method1:arg1] method2:arg2];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]][1];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -16513,6 +16513,11 @@
"  return 42;\n"
"}\n",
Style);
+
+  verifyFormat("if (argc > 5) [[gnu::unused]] {\n"
+   "  return 29;\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LLVMDefaultStyle) {
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseSimpleAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseSimpleAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,51 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+namespace {
+// A class used to set and restore the Token position when peeking
+// ahead in the token source.
+class ScopedTokenPosition {
+  unsigned StoredPosition;
+  FormatTokenSource *Tokens;
+
+public:
+  ScopedTokenPosition(FormatTokenSource *Tokens) : Tokens(Tokens) {
+assert(Tokens && "Tokens expected to not be null");
+StoredPosition = Tokens->getPosition();
+  }
+
+  ~ScopedTokenPosition() { Tokens->setPosition(StoredPosition); }
+};
+} // namespace
+
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseSimpleAttribute() {
+  ScopedTokenPosition AutoPosition(Tokens);
+  FormatToken *Tok = Tokens->getNextToken();
+  // We already read the first [ check for the second.
+  if (Tok && !Tok->is(tok::l_square)) {
+return false;
+  }
+  // Double check that the attribute is just something
+  // fairly simple.
+  while (Tok) {
+if (Tok->is(tok::r_square)) {
+  break;
+}
+Tok = Tokens->getNextToken();
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && !Tok->is(tok::r_square)) {
+return false;
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && Tok->is(tok::semi)) {
+return false;
+  }
+  return true;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1965
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();

rdwampler wrote:
> From the peanut gallery, would checking `TT_AttributeSquare` here work (that 
> should handle ambiguities around ObjC method calls)?  
unfortunately the TT_AttributeSquare hasn't been determined at this stage


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2349
+// ahead in the token source.
+class AutoTokenPosition {
+  unsigned StoredPosition;

nit: I've seen similar RAII idioms in lib/Format use names like [[ 
https://github.com/llvm/llvm-project/blob/6ef45b0426a8c7b9764e102fb1a49561a3a2c118/clang/lib/Format/UnwrappedLineParser.cpp#L139
 | **Scoped**LineState ]] - just wanted to mention that naming alternative.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2372
+  }
+  // Double check  that the attribute is just something
+  // fairly simple.

nit: `check  that` has a double space


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Ronald Wampler via Phabricator via cfe-commits]

rdwampler added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1965
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();

From the peanut gallery, would checking `TT_AttributeSquare` here work (that 
should handle ambiguities around ObjC method calls)?  


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266231.
MyDeveloperDay added a comment.

Add more tests and check if a ';' follows the final ]]


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

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestObjC.cpp

Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,25 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+  verifyFormat("if (argc < 5) [[obj1 method1:arg1] method2:arg2];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]];");
+  verifyFormat("if (argc < 5) [[foo bar] baz:i[0]][1];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -16513,6 +16513,11 @@
"  return 42;\n"
"}\n",
Style);
+
+  verifyFormat("if (argc > 5) [[gnu::unused]] {\n"
+   "  return 29;\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LLVMDefaultStyle) {
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseSimpleAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseSimpleAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,51 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+namespace {
+// A class used to set and restore the Token position when peeking
+// ahead in the token source.
+class AutoTokenPosition {
+  unsigned StoredPosition;
+  FormatTokenSource *Tokens;
+
+public:
+  AutoTokenPosition(FormatTokenSource *Tokens) : Tokens(Tokens) {
+assert(Tokens && "Tokens expected to not be null");
+StoredPosition = Tokens->getPosition();
+  }
+
+  ~AutoTokenPosition() { Tokens->setPosition(StoredPosition); }
+};
+} // namespace
+
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseSimpleAttribute() {
+  AutoTokenPosition AutoPosition(Tokens);
+  FormatToken *Tok = Tokens->getNextToken();
+  // We already read the first [ check for the second
+  if (Tok && !Tok->is(tok::l_square)) {
+return false;
+  }
+  // Double check  that the attribute is just something
+  // fairly simple.
+  while (Tok) {
+if (Tok->is(tok::r_square)) {
+  break;
+}
+Tok = Tokens->getNextToken();
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && !Tok->is(tok::r_square)) {
+return false;
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && Tok->is(tok::semi)) {
+return false;
+  }
+  return true;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D80547#2054763 , @MyDeveloperDay 
wrote:

> Handle more complex nested ObjC calls
>  Rename function to tryParseSimpleAttributes (not supporting full capability 
> as yet)
>  Use RAII object to reset the TokenPosition
>  utilize the fact that Objective calls shouldn't end with "]]" I think... 
> this should allow Attributes at least of the form `[[identifier::identifier]]`
>  I feel if this isn't a good enough perhaps we also check for the `;` after 
> the second `]`


I'm not an ObjC expert, but I am pretty sure you can do something like: `[[foo 
bar] baz: i[0]];` and `[[foo bar] baz: i[0]][1];`


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266204.
MyDeveloperDay added a comment.

Handle more complex nested ObjC calls
Rename function to tryParseSimpleAttributes (not supporting full capability as 
yet)
Use RAII object to reset the TokenPosition
utilize the fact that Objective calls shouldn't end with "]]" I think... this 
should allow Attributes at least of the form `[[identifier::identifier]]`
I feel if this isn't a good enough perhaps we also check for the `;` after the 
second `]`


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

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestObjC.cpp

Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,23 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+  verifyFormat("if (argc < 5) [[obj1 method1:arg1] method2:arg2];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -16513,6 +16513,11 @@
"  return 42;\n"
"}\n",
Style);
+
+  verifyFormat("if (argc > 5) [[gnu::unused]] {\n"
+   "  return 29;\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, LLVMDefaultStyle) {
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseSimpleAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseSimpleAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseSimpleAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,47 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+namespace {
+// A class used to set and restore the Token position when peeking
+// ahead in the token source.
+class AutoTokenPosition {
+  unsigned StoredPosition;
+  FormatTokenSource *Tokens;
+
+public:
+  AutoTokenPosition(FormatTokenSource *Tokens) : Tokens(Tokens) {
+assert(Tokens && "Tokens expected to not be null");
+StoredPosition = Tokens->getPosition();
+  }
+
+  ~AutoTokenPosition() { Tokens->setPosition(StoredPosition); }
+};
+} // namespace
+
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseSimpleAttribute() {
+  AutoTokenPosition AutoPosition(Tokens);
+  FormatToken *Tok = Tokens->getNextToken();
+  // We already read the first [ check for the second
+  if (Tok && !Tok->is(tok::l_square)) {
+return false;
+  }
+  // Double check  that the attribute is just something
+  // fairly simple.
+  while (Tok) {
+if (Tok->is(tok::r_square)) {
+  break;
+}
+Tok = Tokens->getNextToken();
+  }
+  Tok = Tokens->getNextToken();
+  if (Tok && !Tok->is(tok::r_square)) {
+return false;
+  }
+  return true;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced
___
cfe-commits mailing li

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

MyDeveloperDay wrote:
> curdeius wrote:
> > MyDeveloperDay wrote:
> > > MyDeveloperDay wrote:
> > > > This so makes me feel like we need a peekNextToken()
> > > which is like all the time I have to write
> > > 
> > > ```
> > > Tok->Next && Tok->Next->is(tok::XXX)
> > > Tok->Previous && Tok->Previous ->is(tok::XXX)
> > > ```
> > > 
> > > would be so much smaller code if we had
> > > 
> > > ```
> > > Tok->isNext(tok::XXX)
> > > Tok->isPrevious(tok::XXX)
> > > ```
> > `peekNextToken()` probably should be done in a separate revision, nope?
> > It would be good to have it IMO.
> yes I was just thinking out loud..
I think this should be more strict and check for the sequence of 5 tokens: 
```
tok::l_square, tok::l_square, tok::identifier, tok::r_square, tok::r_square
```

Unfortunately `[[` may start a nested Objective-C-style method call, e.g., 
```
[[obj1 method1:arg1] method2:arg2]
```
(I'm not super familiar with Objective-C-syntax, please correct me if I'm wrong 
about that.)

(Only checking for the last `]]`, or a combination of `[[` and `]]` that 
doesn't examine //the meat in-between// would suffer from similar ambiguities.)

Of course, attributes can be more complicated and can have a rich internal 
structure (looking at https://en.cppreference.com/w/cpp/language/attributes). 
Consider renaming this to `tryToParseSimpleAttribute` to not give folks false 
hopes that this deals with the general problem for now (we can rename this 
later as we improve this C++ attribute parsing to handle more of the 
interesting cases).



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2351
+  FormatToken *Tok = Tokens->getNextToken();
+  FormatTok = Tokens->setPosition(StoredPosition);
+  if (Tok->is(tok::l_square)) {

curdeius wrote:
> Maybe add `assert(Tok);`. How can you be know for sure that there's a next 
> token?
+1, but instead of `assert`-ing which may/will crash clang-format on weird 
inputs and may lead to UB below in release builds, just `return false` and move 
on.


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 266192.
MyDeveloperDay added a comment.

Check for null Tok
Fix up comments


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

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTestObjC.cpp


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,22 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,18 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+// Look to see if we have [[ by looking ahead, if
+// its not then rewind to the original position.
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();
+  FormatToken *Tok = Tokens->getNextToken();
+  FormatTok = Tokens->setPosition(StoredPosition);
+  if (Tok && Tok->is(tok::l_square)) {
+return true;
+  }
+  return false;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,22 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+
+  verifyFormat("if (argc < 5) [[unlikely]]\n"
+   "  [obj func:arg];\n"
+   "else [[likely]]\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

curdeius wrote:
> MyDeveloperDay wrote:
> > MyDeveloperDay wrote:
> > > This so makes me feel like we need a peekNextToken()
> > which is like all the time I have to write
> > 
> > ```
> > Tok->Next && Tok->Next->is(tok::XXX)
> > Tok->Previous && Tok->Previous ->is(tok::XXX)
> > ```
> > 
> > would be so much smaller code if we had
> > 
> > ```
> > Tok->isNext(tok::XXX)
> > Tok->isPrevious(tok::XXX)
> > ```
> `peekNextToken()` probably should be done in a separate revision, nope?
> It would be good to have it IMO.
yes I was just thinking out loud..


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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Minor remarks.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2346
 
+// look to see if we have [[ by looking ahead if
+// its not then rewind to the original position

Nit: shouldn't comments be "Full phrases."?



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > This so makes me feel like we need a peekNextToken()
> which is like all the time I have to write
> 
> ```
> Tok->Next && Tok->Next->is(tok::XXX)
> Tok->Previous && Tok->Previous ->is(tok::XXX)
> ```
> 
> would be so much smaller code if we had
> 
> ```
> Tok->isNext(tok::XXX)
> Tok->isPrevious(tok::XXX)
> ```
`peekNextToken()` probably should be done in a separate revision, nope?
It would be good to have it IMO.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2351
+  FormatToken *Tok = Tokens->getNextToken();
+  FormatTok = Tokens->setPosition(StoredPosition);
+  if (Tok->is(tok::l_square)) {

Maybe add `assert(Tok);`. How can you be know for sure that there's a next 
token?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

MyDeveloperDay wrote:
> This so makes me feel like we need a peekNextToken()
which is like all the time I have to write

```
Tok->Next && Tok->Next->is(tok::XXX)
Tok->Previous && Tok->Previous ->is(tok::XXX)
```

would be so much smaller code if we had

```
Tok->isNext(tok::XXX)
Tok->isPrevious(tok::XXX)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2348
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();

This so makes me feel like we need a peekNextToken()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80547



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

[PATCH] D80547: [clang-format] Fix an ObjC regression introduced with new [[likely]][[unlikely]] support in if/else clauses

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: krasimir, JakeMerdichAMD.
MyDeveloperDay added projects: clang, clang-format.

D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] 
but we forgot to teach clang-format  introduce 
an ObjC regression

Only parse the `[]` if what follows is really an attribute


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80547

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTestObjC.cpp


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,18 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+  Style.ObjCBreakBeforeNestedBlockParam = false;
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,7 +1981,7 @@
   if (FormatTok->Tok.is(tok::kw_else)) {
 nextToken();
 // handle [[likely]] / [[unlikely]]
-if (FormatTok->is(tok::l_square))
+if (FormatTok->Tok.is(tok::l_square) && tryToParseAttribute())
   parseSquare();
 if (FormatTok->Tok.is(tok::l_brace)) {
   CompoundStatementIndenter Indenter(this, Style, Line->Level);
@@ -2343,6 +2343,18 @@
   // "} n, m;" will end up in one unwrapped line.
 }
 
+// look to see if we have [[ by looking ahead if
+// its not then rewind to the original position
+bool UnwrappedLineParser::tryToParseAttribute() {
+  unsigned StoredPosition = Tokens->getPosition();
+  FormatToken *Tok = Tokens->getNextToken();
+  FormatTok = Tokens->setPosition(StoredPosition);
+  if (Tok->is(tok::l_square)) {
+return true;
+  }
+  return false;
+}
+
 void UnwrappedLineParser::parseJavaEnumBody() {
   // Determine whether the enum is simple, i.e. does not have a semicolon or
   // constants with class bodies. Simple enums can be formatted like braced


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1434,6 +1434,18 @@
   "   }]");
 }
 
+TEST_F(FormatTestObjC, IfNotUnlikely) {
+  Style = getGoogleStyle(FormatStyle::LK_ObjC);
+  Style.ObjCBreakBeforeNestedBlockParam = false;
+
+  verifyFormat("if (argc < 5) [obj func:arg];");
+
+  verifyFormat("if (argc < 5)\n"
+   "  [obj func:arg];\n"
+   "else\n"
+   "  [obj func:arg2];");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -134,6 +134,7 @@
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
+  bool tryToParseAttribute();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1962,7 +1962,7 @@
   if (FormatTok->Tok.is(tok::l_paren))
 parseParens();
   // handle [[likely]] / [[unlikely]]
-  if (FormatTok->is(tok::l_square))
+  if (FormatTok->is(tok::l_square) && tryToParseAttribute())
 parseSquare();
   bool NeedsUnwrappedLine = false;
   if (FormatTok->Tok.is(tok::l_brace)) {
@@ -1981,