Page MenuHomePhabricator

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

Authored by MyDeveloperDay on May 26 2020, 4:11 AM.

Diff Detail

Event Timeline

MyDeveloperDay marked an inline comment as done.May 26 2020, 4:20 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2348

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

MyDeveloperDay marked an inline comment as done.May 26 2020, 4:22 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2348

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)

Minor remarks.

clang/lib/Format/UnwrappedLineParser.cpp
2346

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

2348

peekNextToken() probably should be done in a separate revision, nope?
It would be good to have it IMO.

2351

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

MyDeveloperDay marked an inline comment as done.May 26 2020, 6:15 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2348

yes I was just thinking out loud..

Check for null Tok
Fix up comments

krasimir added inline comments.May 26 2020, 6:22 AM
clang/lib/Format/UnwrappedLineParser.cpp
2348

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).

2351

+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.

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 ]

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];

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

MyDeveloperDay marked 5 inline comments as done.May 26 2020, 8:33 AM
rdwampler added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
1965

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

krasimir added inline comments.May 26 2020, 8:55 AM
clang/lib/Format/UnwrappedLineParser.cpp
2349

nit: I've seen similar RAII idioms in lib/Format use names like **Scoped**LineState - just wanted to mention that naming alternative.

2372

nit: check that has a double space

MyDeveloperDay marked 2 inline comments as done.May 26 2020, 9:14 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
1965

unfortunately the TT_AttributeSquare hasn't been determined at this stage

MyDeveloperDay marked an inline comment as done.

Address nits

MyDeveloperDay marked 2 inline comments as done.May 26 2020, 9:26 AM
krasimir accepted this revision.May 26 2020, 10:14 AM

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.

This revision is now accepted and ready to land.May 26 2020, 10:14 AM
This revision was automatically updated to reflect the committed changes.