[PATCH] D43906: [clang-format] Improve detection of Objective-C block types

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> Would it be enough to only add the block type case? With the macro false positive, there won't be an open paren after the closing paren, right?

Are you asking to remove the block variable cases? I was concerned those would no longer be handled correctly if we don't explicitly check for them.

What change are you suggesting if we remove the block variable cases to handle those?



================
Comment at: lib/Format/TokenAnnotator.cpp:155
+           Next->startsSequence(tok::identifier, tok::l_square,
+                                tok::numeric_constant, tok::r_square,
+                                tok::r_paren, tok::l_paren))) {
----------------
djasper wrote:
> This seems suspect. Does it have to be a numeric_constant?
Probably not, any constexpr would do, I suspect. What's the best way to parse that?


================
Comment at: unittests/Format/FormatTest.cpp:12027
 
+TEST_F(FormatTest, GuessLanguageWithCaret) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
----------------
krasimir wrote:
> Please also add formatting tests. This might require changes to the formatting logic since I'd intuitively expect ``int(^)(char)`` to be formatted as ``int (^)(char)``.
Added formatting tests. Tests passed with no changes to the formatting logic, and your intuition was correct.


================
Comment at: unittests/Format/FormatTest.cpp:12029
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h", "FOO(^);"));
+  EXPECT_EQ(FormatStyle::LK_ObjC,
+            guessLanguage("foo.h", "int(^)(char, float);"));
----------------
djasper wrote:
> I am somewhat skeptical about all these tests now being solely about guessLanguage. It might be the best choice for some of them, but also, there might be other things we do to detect ObjC at some point and then all of these tests become meaningless.
> 
> Does your change create a different formatting here?
I hear you. I just want to make sure the false positive case doesn't regress, and we don't lose the functionality of detecting ObjC for block types.

Added formatting tests.


Repository:
  rC Clang

https://reviews.llvm.org/D43906