[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