[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 558132.
jaredgrubb marked 3 inline comments as done.
jaredgrubb added a comment.

Update to address all the review comments.


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,423 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+  "nonatomic",
+  "assign",
+  "retain",
+  "strong",
+  "copy",
+  "weak",
+  "unsafe_unretained",
+  "readonly",
+  "readwrite",
+  "getter",
+  "setter",
+  "nullable",
+  "nonnull",
+  "null_resettable",
+  "null_unspecified",
+  }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Zero: nothing to do, but is legal.
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+
+  // Check that properties preceded by @optional/@required work.
+  verifyFormat("@optional\n"
+   "@property(a, b) int p;",
+  

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb marked 7 inline comments as done.
jaredgrubb added inline comments.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),

owenpan wrote:
> jaredgrubb wrote:
> > owenpan wrote:
> > > Is it valid in Objective-C to have duplicate attributes?
> > It's silly, but clang doesn't seem to care. I tried this, so duplicate was 
> > ok, but contradiction was flagged:
> > ```
> > @property (strong, nonatomic, nonatomic) X *X;   // duplicate, but no error
> > @property (strong, nonatomic, weak) Y *y;   // error: Property attributes 
> > 'strong' and 'weak' are mutually exclusive
> > ```
> > 
> > I wasn't sure whether to do this, but went that way since that's what "sort 
> > include files" did. However, it seems like an odd corner case so I'd be ok 
> > removing this uniquing if you prefer.
> We should keep the duplicates if clang accepts them. What happens to e.g. 
> `@property(x=a, y=b, x=a, y=c)`?
Looks like clang does not reject duplicates like that either, even though I 
can't imagine that it results in anything sane:
```
@property (getter=Foo, getter=Bar) int x;
```

I'll remove the de-dup code as you request -- it'll make the pass simpler and 
marginally faster :)

For dup-cases where the values differ (eg, `y=b, y=c`), the pass will just 
stable-sort them. This is the simplest thing to do and IMO is good-enough as I 
think dup's are questionable anyway.

I've added some test cases for this (and removed the original de-dup test 
cases).

I'll add a couple unit tests to confirm this and include your example.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+  for (AnnotatedLine *Line : AnnotatedLines) {
+if (!Line->Affected || Line->InPPDirective)
+  continue;

owenpan wrote:
> jaredgrubb wrote:
> > owenpan wrote:
> > > Why not `InPPDirective`?
> > I copy-pasted this from `LeftRightQualifierAlignmentFixer::analyze`, which 
> > I used as a template since I'm still getting used to the codebase. I wasn't 
> > sure whether this was important, so I left it in. But I don't think I have 
> > a good reason. 
> > 
> > I've added a new test case `SortsInPPDirective` that spot-checks some macro 
> > definition examples (which did fail unless this `Line->InPPDirective` check 
> > was removed, as expected.).
> What about `Line->Type != LT_ObjCProperty` I suggested?
Ah, I missed that suggestion. 

I don't have enough understanding of clang-format to say whether that would be 
correct, but I'll trust you :) 

When I add this check it doesn't break unit tests or cause any 
change-in-behavior on my ObjC test repo (~10K .h/.m files). So, it -seems- like 
this is a valid check to add, so I'll do it!



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+  continue;
+
+const auto *Last = Line->Last;

owenpan wrote:
> jaredgrubb wrote:
> > owenpan wrote:
> > > Must `@property` be the first non-comment tokens of an unwrapped line in 
> > > Objective-C? And at most one `@property` per line?
> > I can't think of any token that should precede a `@property`. Aka, I don't 
> > think there are any `__attribute__` that can fill that slot. 
> > 
> > You could have `@optional` or `@required` floating around between 
> > property/method declarations. I've added a test-case for a `@property` that 
> > is preceded by these tokens and proved that the reordering is handled 
> > correctly.
> > 
> > As for multiple properties in one line, I've never seen a codebase with 
> > that. clang-format does split them already.
> I asked the questions because if yes, we could move on to the next line 
> before hitting the last token of the current line.
In testing, it seems that some other pass (not sure which) will split the line 
before this new pass sees it. So if I can rely on that (I think I can?), then 
we can early-break like you propose.

I've added a unit test to cover this scenario.


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

In D150083#4656832 , @owenpan wrote:

> Thank you for your patience!

I appreciate the help :) I'm excited to get this in!

> In D150083#4655528 , @owenpan wrote:
>
>> See also D153228 .
>
> Would this patch have a similar performance issue?

Reading through the issue, I don't think so. As I understand that issue, the 
scale-problem is due to how the passes interact with one another as a whole 
(since that setting creates a Pass for _each_ qualifier). This ObjC pass uses 
only ONE pass to handle the ordering of all property attributes. I 
intentionally did not copy that part of the Qualifier pass because it didn't 
seem to actually help my use-case, which is much narrower (primarily that I 
only have to operate on lines that look like properties, not in arbitrary 
places like function args and variable declarations, etc.).

Please correct me if I missed something about that patch that could apply.


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

From my perspective, this patch is ready to go! Please let me know if there's 
anything more I can do to improve the patch! (@owenpan thanks so much for your 
help so far!)


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 558010.
jaredgrubb added a comment.

Adjusting the Style comments, since the behavior changed and were not updated 
to reflect that leading/trailing comments no longer have special handling.


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,412 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+  "nonatomic",
+  "assign",
+  "retain",
+  "strong",
+  "copy",
+  "weak",
+  "unsafe_unretained",
+  "readonly",
+  "readwrite",
+  "getter",
+  "setter",
+  "nullable",
+  "nonnull",
+  "null_resettable",
+  "null_unspecified",
+  }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Zero: nothing to do, but is legal.
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+
+  // Check that properties preceded by @optional/@required work.
+  

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

I opened https://github.com/llvm/llvm-project/issues/71323 for this patch.


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 558009.
jaredgrubb marked 28 inline comments as done.
jaredgrubb added a comment.

Addressing all review comments to date.


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,412 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+  "nonatomic",
+  "assign",
+  "retain",
+  "strong",
+  "copy",
+  "weak",
+  "unsafe_unretained",
+  "readonly",
+  "readwrite",
+  "getter",
+  "setter",
+  "nullable",
+  "nonnull",
+  "null_resettable",
+  "null_unspecified",
+  }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.Language = FormatStyle::LK_ObjC;
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  // Zero: nothing to do, but is legal.
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+
+  // Check that properties preceded by @optional/@required work.
+  verifyFormat("@optional\n"
+   "@property(a, b) int p;",
+

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:55
+const FormatToken *LParenTok, const FormatToken *RParenTok) const {
+  // Skip past any leading comments.
+  const FormatToken *const BeginTok = LParenTok->getNextNonComment();

owenpan wrote:
> I strongly suggest that we bail out as well if there are leading and/or 
> trailing comments.
I'm ok with that!

In our codebase we have some code like this:
```
@property (/*nullable*/ readonly, strong) Thing *thing;
```
Nullablility annotations are all-or-nothing in a header file (putting it once 
triggers clang to complain about it missing in other eligible places). We have 
places where a dev wanted to add the attribute to one line but didn't want to 
commit to auditing the whole header. Their comment is a hint to the lucky 
person who does that work one day.

I thought it would be easy to handle a leading/trailing comment in this patch, 
but I am very ok with skipping these lines completely as you suggest. 
Conservative is probably safer!

I've revised the tests to affirm that any comments get skipped now.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:77
+  // Memoize the attribute. (Note that 'class' is a legal attribute!)
+  PropertyAttributes.push_back({Tok->TokenText.trim(), StringRef{}});
+

owenpan wrote:
> Do we need `trim()`?
I removed both `trim()` and my tests still pass. I wasn't fully sure when 
`trim` would help, but it doesn't seem to in this case.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:97
+  }
+
+  // Create a "remapping index" on how to reorder the attributes.

owenpan wrote:
> Can we assert `PropertyAttributes.size() > 1` here?
We shouldn't _assert_, as it is valid to have just one. But I can add an 
early-return for that though. (I'll also early-return if it's zero, which is 
also legal, eg `@property () int x`)



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:113
+
+  // Deduplicate the attributes.
+  Indices.erase(std::unique(Indices.begin(), Indices.end(),

owenpan wrote:
> Is it valid in Objective-C to have duplicate attributes?
It's silly, but clang doesn't seem to care. I tried this, so duplicate was ok, 
but contradiction was flagged:
```
@property (strong, nonatomic, nonatomic) X *X;   // duplicate, but no error
@property (strong, nonatomic, weak) Y *y;   // error: Property attributes 
'strong' and 'weak' are mutually exclusive
```

I wasn't sure whether to do this, but went that way since that's what "sort 
include files" did. However, it seems like an odd corner case so I'd be ok 
removing this uniquing if you prefer.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:120
+Indices.end());
+
+  // If there are no removals or shuffling, then don't suggest any fixup.

owenpan wrote:
> Is it possible that `Indices` becomes empty or has only one element after 
> deduplication? 
It is possible to reduce to one element (I have a unit test case for that 
scenario `(a, a)`).

I don't see how `std::unique` could take a non-empty list and output an empty 
list, so I'll claim "no" on the empty part.



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:184
+  for (AnnotatedLine *Line : AnnotatedLines) {
+if (!Line->Affected || Line->InPPDirective)
+  continue;

owenpan wrote:
> Why not `InPPDirective`?
I copy-pasted this from `LeftRightQualifierAlignmentFixer::analyze`, which I 
used as a template since I'm still getting used to the codebase. I wasn't sure 
whether this was important, so I left it in. But I don't think I have a good 
reason. 

I've added a new test case `SortsInPPDirective` that spot-checks some macro 
definition examples (which did fail unless this `Line->InPPDirective` check was 
removed, as expected.).



Comment at: clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp:190
+  continue;
+
+const auto *Last = Line->Last;

owenpan wrote:
> Must `@property` be the first non-comment tokens of an unwrapped line in 
> Objective-C? And at most one `@property` per line?
I can't think of any token that should precede a `@property`. Aka, I don't 
think there are any `__attribute__` that can fill that slot. 

You could have `@optional` or `@required` floating around between 
property/method declarations. I've added a test-case for a `@property` that is 
preceded by these tokens and proved that the reordering is handled correctly.

As for multiple properties in one line, I've never seen a codebase with that. 
clang-format does split them already.


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

https://reviews.llvm.org/D150083

___
cfe-commits mailing list

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

Should I open a GitHub issue for this patch?


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 557922.
jaredgrubb added a comment.

Address review feedback from @owenpan on Oct23. Thanks for helping me tune this 
patch!


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,393 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+  "nonatomic",
+  "assign",
+  "retain",
+  "strong",
+  "copy",
+  "weak",
+  "unsafe_unretained",
+  "readonly",
+  "readwrite",
+  "getter",
+  "setter",
+  "nullable",
+  "nonnull",
+  "null_resettable",
+  "null_unspecified",
+  }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsAttributesWithValues) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ObjCPropertyAttributeOrder = {"a", "getter", "c"};
+
+  // No change
+  

[PATCH] D145262: [clang-format] Treat AttributeMacros more like __attribute__

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

In D145262#4653670 , @owenpan wrote:

> @jaredgrubb please provide your authorship for `git commit --author` so that 
> we can land the patch on your behalf.

The best to use would be:

  Jared Grubb 

Thanks so much for your help on this!!


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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like __attribute__

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 557674.
jaredgrubb added a comment.

Addressed review feedback from Oct2 (@owenpan).
A new issue on Github was opened for this enhancement 
(https://github.com/llvm/llvm-project/issues/68722)


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1795,6 +1795,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 557542.
jaredgrubb added a comment.

Address review comments, and adjusting the patch to address other merges since 
this patch was started.


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1770,6 +1770,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeRParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeLParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeRParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("@property(weak) id delegate A(X);");

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

@MyDeveloperDay addressed your comments. Thanks!

Would love to get this in before Phabricator closes. Anything else I can do to 
help?


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

@MyDeveloperDay addressed your comments. Thanks!

Would love to get this in before Phabricator closes. Anything else I can do to 
help?


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 557275.
jaredgrubb added a comment.

Rebased and adjusted docs to reflect that this patch would appear in 
clang-format 18 (not 17 now).
Removed extraneous comment change (will do NFC later for that)
Removed other build-system changes (as requested).


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,393 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+  "nonatomic",
+  "assign",
+  "retain",
+  "strong",
+  "copy",
+  "weak",
+  "unsafe_unretained",
+  "readonly",
+  "readwrite",
+  "getter",
+  "setter",
+  "nullable",
+  "nonnull",
+  "null_resettable",
+  "null_unspecified",
+  }));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  verifyFormat("@property(b) int p;", Style);
+  verifyFormat("@property(c) int p;", Style);
+
+  // Two in correct order already: no change.
+  verifyFormat("@property(a, b) int p;", Style);
+  verifyFormat("@property(a, c) int p;", Style);
+  verifyFormat("@property(b, c) int p;", Style);
+
+  // Three in correct order already: no change.
+  verifyFormat("@property(a, b, c) int p;", Style);
+
+  // Two wrong order.
+  verifyFormat("@property(a, b) int p;", "@property(b, a) int p;", Style);
+  verifyFormat("@property(a, c) int p;", "@property(c, a) int p;", Style);
+  verifyFormat("@property(b, c) int p;", "@property(c, b) int p;", Style);
+
+  // Three wrong order.
+  verifyFormat("@property(a, b, c) int p;", "@property(b, a, c) int p;", Style);
+  verifyFormat("@property(a, b, c) int p;", "@property(c, b, a) int p;", Style);
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Format/FormatTestObjC.cpp:1619
+  // Reflow after first macro.
+  // FIXME: these should indent but don't.
+  verifyFormat("- (id)init ATTRIBUTE_MACRO(X)\n"

owenpan wrote:
> jaredgrubb wrote:
> > I don't love this FIXME, but I was afraid to add more to this patch, as 
> > fixing this will require digging into things that have nothing to do with 
> > `__attribute__` vs `AttributeMacros`.
> > 
> > For example, suffix macros in C/C++ also are broken in the same way with 
> > just plain `__attribute__`. For example, for `ColumnWidth: 50`:
> > ```
> > int f(double) __attribute__((overloadable))
> > __attribute__((overloadable));
> > 
> > int ff(double)
> > __attribute__((overloadable))
> > __attribute__((overloadable));
> > ```
> > 
> > I think fixing reflowing of suffix macros is best done in another PR (which 
> > I can take a stab at!)
> Half of the test cases passed before this patch but now would fail with this 
> patch. That is, this patch would generate regressions.
Just saw this comment. Yes, 3 of these did pass, but lots more in this file do 
NOT pass. I understand the desire to not "regress", but this patch improves so 
many other examples (as documented in these test cases). I can pick some of the 
worst if it helps, but otherwise, I'm not sure what I can do to address your 
comment.


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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 557274.
jaredgrubb added a comment.

Hopefully this will satisfy and we can merge! I didn't notice the Phabricator 
timeline and I'm hoping we can finish this before Oct 1.

Rebased.
Removed extraneous doc comment not related to patch (as requested).
Removed comment (as requested).
Added a `#if 0` around some cases that should be fixed one day (as requested 
prior but I missed one of the chunks).


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

https://reviews.llvm.org/D145262

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1731,6 +1731,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], 

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 540492.
jaredgrubb added a comment.

Rebased (no other updates) to re-run build and hopefully get some eyes on this.


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
  llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
  llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1357,6 +1357,7 @@
 "lib/Format/FormatTokenLexer.h",
 "lib/Format/FormatTokenSource.h",
 "lib/Format/Macros.h",
+"lib/Format/ObjCPropertyAttributeOrderFixer.h",
 "lib/Format/QualifierAlignmentFixer.h",
 "lib/Format/UnwrappedLineParser.h",
 ] + glob([
Index: llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
@@ -36,6 +36,7 @@
 "MacroCallReconstructorTest.cpp",
 "MacroExpanderTest.cpp",
 "NamespaceEndCommentsFixerTest.cpp",
+"ObjCPropertyAttributeOrderFixerTest.cpp",
 "QualifierFixerTest.cpp",
 "SortImportsTestJS.cpp",
 "SortImportsTestJava.cpp",
Index: llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
@@ -20,6 +20,7 @@
 "MacroCallReconstructor.cpp",
 "MacroExpander.cpp",
 "NamespaceEndCommentsFixer.cpp",
+"ObjCPropertyAttributeOrderFixer.cpp",
 "QualifierAlignmentFixer.cpp",
 "SortJavaScriptImports.cpp",
 "TokenAnalyzer.cpp",
Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,393 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",
+  "direct",
+  "atomic",
+ 

[PATCH] D146434: [clang-format] Fix support for ObjC blocks with pointer return types

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 540488.
jaredgrubb marked an inline comment as done.
jaredgrubb added a comment.

Address review comment and rebase to re-run tests. Intend to merge if 
everything is green! (If there are further comments, I will commit to address 
them!)


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

https://reviews.llvm.org/D146434

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -1019,6 +1019,20 @@
   verifyFormat("int (^foo[kNumEntries])(char, float);");
   verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
   verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+
+  verifyFormat("int *p = ^int *() { //\n"
+   "  return nullptr;\n"
+   "}();");
+
+  verifyFormat("int * (^p)(void) = ^int *(void) { //\n"
+   "  return nullptr;\n"
+   "};");
+
+  // WebKit forces function braces onto a newline, but blocks should not.
+  verifyFormat("int* p = ^int*() { //\n"
+   "return nullptr;\n"
+   "}();",
+   getWebKitStyle());
 }
 
 TEST_F(FormatTestObjC, ObjCSnippets) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22619,7 +22619,8 @@
"}\n"
"  }\n"
"});");
-  verifyFormat("Block b = ^int *(A *a, B *b) {}");
+  verifyFormat("Block b = ^int *(A *a, B *b) {\n"
+   "};");
   verifyFormat("BOOL (^aaa)(void) = ^BOOL {\n"
"};");
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1798,12 +1798,18 @@
   break;
 case tok::caret:
   nextToken();
+  // Block return type.
   if (FormatTok->Tok.isAnyIdentifier() ||
   FormatTok->isSimpleTypeSpecifier()) {
 nextToken();
+// Return types: pointers are ok too.
+while (FormatTok->is(tok::star))
+  nextToken();
   }
+  // Block argument list.
   if (FormatTok->is(tok::l_paren))
 parseParens();
+  // Block body.
   if (FormatTok->is(tok::l_brace))
 parseChildBlock();
   break;


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1019,6 +1019,20 @@
   verifyFormat("int (^foo[kNumEntries])(char, float);");
   verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
   verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+
+  verifyFormat("int *p = ^int *() { //\n"
+   "  return nullptr;\n"
+   "}();");
+
+  verifyFormat("int * (^p)(void) = ^int *(void) { //\n"
+   "  return nullptr;\n"
+   "};");
+
+  // WebKit forces function braces onto a newline, but blocks should not.
+  verifyFormat("int* p = ^int*() { //\n"
+   "return nullptr;\n"
+   "}();",
+   getWebKitStyle());
 }
 
 TEST_F(FormatTestObjC, ObjCSnippets) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22619,7 +22619,8 @@
"}\n"
"  }\n"
"});");
-  verifyFormat("Block b = ^int *(A *a, B *b) {}");
+  verifyFormat("Block b = ^int *(A *a, B *b) {\n"
+   "};");
   verifyFormat("BOOL (^aaa)(void) = ^BOOL {\n"
"};");
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1798,12 +1798,18 @@
   break;
 case tok::caret:
   nextToken();
+  // Block return type.
   if (FormatTok->Tok.isAnyIdentifier() ||
   FormatTok->isSimpleTypeSpecifier()) {
 nextToken();
+// Return types: pointers are ok too.
+while (FormatTok->is(tok::star))
+  nextToken();
   }
+  // Block argument list.
   if (FormatTok->is(tok::l_paren))
 parseParens();
+  // Block body.
   if (FormatTok->is(tok::l_brace))
 parseChildBlock();
   break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146434: [clang-format] Fix support for ObjC blocks with pointer return types

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb marked 2 inline comments as done.
jaredgrubb added inline comments.
Herald added a subscriber: wangpc.
Herald added a reviewer: rymiel.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1779-1782
 nextToken();
+// Return types: pointers are ok too.
+while (FormatTok->is(tok::star))
+  nextToken();

owenpan wrote:
> 
I think this obscures the logic a bit. The first one is to consume one token 
(the return type) and the next is to consume a different kind of token 
(trailing stars). Putting them in a while loop makes it harder to reason about 
why it's looping in that way.



Comment at: clang/unittests/Format/FormatTestObjC.cpp:1007-1010
+  Style = getWebKitStyle();
+  verifyFormat("int* p = ^int*() { //\n"
+   "return nullptr;\n"
+   "}();");

owenpan wrote:
> 
Fixed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146434

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 540477.
jaredgrubb added a comment.

Rebased and added a line to Release Notes, no other changes. Have requested 
this to be merged if all builds are green (since this patch has been approved).


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

https://reviews.llvm.org/D145262

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1642,6 +1642,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeParen);
+
+  // 

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

Are there any other comments on this patch? I would love to make this into 
clang-17!


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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 524351.
jaredgrubb marked an inline comment as done.
jaredgrubb added a comment.

Address review comments: unroll a loop in unit tests to explicitly test all the 
property attributes.


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
  llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
  llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1301,6 +1301,7 @@
 "lib/Format/FormatTokenLexer.h",
 "lib/Format/FormatTokenSource.h",
 "lib/Format/Macros.h",
+"lib/Format/ObjCPropertyAttributeOrderFixer.h",
 "lib/Format/QualifierAlignmentFixer.h",
 "lib/Format/UnwrappedLineParser.h",
 ] + glob([
Index: llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
@@ -36,6 +36,7 @@
 "MacroCallReconstructorTest.cpp",
 "MacroExpanderTest.cpp",
 "NamespaceEndCommentsFixerTest.cpp",
+"ObjCPropertyAttributeOrderFixerTest.cpp",
 "QualifierFixerTest.cpp",
 "SortImportsTestJS.cpp",
 "SortImportsTestJava.cpp",
Index: llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
@@ -20,6 +20,7 @@
 "MacroCallReconstructor.cpp",
 "MacroExpander.cpp",
 "NamespaceEndCommentsFixer.cpp",
+"ObjCPropertyAttributeOrderFixer.cpp",
 "QualifierAlignmentFixer.cpp",
 "SortJavaScriptImports.cpp",
 "TokenAnalyzer.cpp",
Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,393 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: ["
+  "class, direct, atomic, nonatomic, "
+  "assign, retain, strong, copy, weak, unsafe_unretained, "
+  "readonly, readwrite, getter, setter, "
+  "nullable, nonnull, null_resettable, null_unspecified"
+  "]",
+  ObjCPropertyAttributeOrder,
+  std::vector({
+  "class",

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 521953.
jaredgrubb added a comment.

Address review comments:

- remove redundant `&&`
- remove part of patch that was not tested by any test and should be its own 
patch on its own merits


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1543,6 +1543,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

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



Comment at: clang/lib/Format/TokenAnnotator.cpp:5473
+  if (Right.isOneOf(tok::kw___attribute, TT_AttributeMacro))
+return true;
+

HazardyKnusperkeks wrote:
> jaredgrubb wrote:
> > HazardyKnusperkeks wrote:
> > > Does changing this return value make no difference? In other words is 
> > > there no combination of `Left.is(TT_AttributeSquare)` and 
> > > `Right.is(tok::kw___attribute)`?
> > Yes, that combo can happen; example below.
> > 
> > The patch that changed the left-diff line from `true` to 
> > `!Left.is(TT_AttributeSquare)`
> >  was done _only_ contemplating the `[[` case 
> > (5a4ddbd69db2b0e09398214510501d0e59a0c30b); tagging @MyDeveloperDay who 
> > wrote that patch and can perhaps offer more insight on your question.
> > 
> > My reasoning is to revert that part of the patch just a bit and limit it to 
> > that case only.
> > 
> > I couldn't come up with a use-case where you'd want to avoid splitting 
> > between `TT_AttributeSquare` and `kw___attribute`, but the example below 
> > shows that allowing it to break in that combination is preferable to the 
> > alternative of breaking in the parens of the attribute:
> > ```
> > // Style: "{BasedOnStyle: LLVM, ColumnLimit: 40}"
> > // Existing Behavior
> > int ff(
> > double)
> > __attribute__((overloadable))
> > [[unused]] __attribute__((
> > overloadable));
> > 
> > // With Patch
> > int ff(
> > double)
> > __attribute__((overloadable))
> > [[unused]]
> > __attribute__((overloadable));
> > ```
> > 
> Thanks for the detailed answer. I'll wait for @MyDeveloperDay. You are right, 
> it's prettier with the patch, but on the other hand it is not desirable to 
> change the formatting (apart from fixing bugs) between versions.
This part of the patch isn't necessary so let me revert it so it doesn't slow 
this patch down.

But, the unit tests don't even fail when I revert it -- which means I really 
should treat this part as its own change with a unit test that matters. 


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

https://reviews.llvm.org/D145262

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 521952.
jaredgrubb added a comment.

Address review comments:

- fix some style
- add unit test for each ObjC attribute recognized by the compiler
- adjust the docs for the style-option to show a YAML example with all of them 
in a sane order (something people could copy-paste as a starter version)


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
  llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
  llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1301,6 +1301,7 @@
 "lib/Format/FormatTokenLexer.h",
 "lib/Format/FormatTokenSource.h",
 "lib/Format/Macros.h",
+"lib/Format/ObjCPropertyAttributeOrderFixer.h",
 "lib/Format/QualifierAlignmentFixer.h",
 "lib/Format/UnwrappedLineParser.h",
 ] + glob([
Index: llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
@@ -36,6 +36,7 @@
 "MacroCallReconstructorTest.cpp",
 "MacroExpanderTest.cpp",
 "NamespaceEndCommentsFixerTest.cpp",
+"ObjCPropertyAttributeOrderFixerTest.cpp",
 "QualifierFixerTest.cpp",
 "SortImportsTestJS.cpp",
 "SortImportsTestJava.cpp",
Index: llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
@@ -20,6 +20,7 @@
 "MacroCallReconstructor.cpp",
 "MacroExpander.cpp",
 "NamespaceEndCommentsFixer.cpp",
+"ObjCPropertyAttributeOrderFixer.cpp",
 "QualifierAlignmentFixer.cpp",
 "SortJavaScriptImports.cpp",
 "TokenAnalyzer.cpp",
Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,207 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+
+  static std::vector getAllObjCAttributes() {
+// These are all the ObjC property attributes that are currently supported in ObjC.
+// The Fixer doesn't actually know these, it just accepts whatever tokens the user provides.
+// These are specified here just to be exhaustive on the tokens that are expected, and to 
+// make sure they are handled correctly. For example, 'class' is a keyword, so it could
+// get trapped in an unexpected way.
+return { 
+  "class", "direct", "atomic", "nonatomic",
+  "assign", "retain", "strong", "copy", "weak", "unsafe_unretained",
+  "readonly", "readwrite", 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:5473
+  if (Right.isOneOf(tok::kw___attribute, TT_AttributeMacro))
+return true;
+

HazardyKnusperkeks wrote:
> Does changing this return value make no difference? In other words is there 
> no combination of `Left.is(TT_AttributeSquare)` and 
> `Right.is(tok::kw___attribute)`?
Yes, that combo can happen; example below.

The patch that changed the left-diff line from `true` to 
`!Left.is(TT_AttributeSquare)`
 was done _only_ contemplating the `[[` case 
(5a4ddbd69db2b0e09398214510501d0e59a0c30b); tagging @MyDeveloperDay who wrote 
that patch and can perhaps offer more insight on your question.

My reasoning is to revert that part of the patch just a bit and limit it to 
that case only.

I couldn't come up with a use-case where you'd want to avoid splitting between 
`TT_AttributeSquare` and `kw___attribute`, but the example below shows that 
allowing it to break in that combination is preferable to the alternative of 
breaking in the parens of the attribute:
```
// Style: "{BasedOnStyle: LLVM, ColumnLimit: 40}"
// Existing Behavior
int ff(
double)
__attribute__((overloadable))
[[unused]] __attribute__((
overloadable));

// With Patch
int ff(
double)
__attribute__((overloadable))
[[unused]]
__attribute__((overloadable));
```



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

https://reviews.llvm.org/D145262

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 520247.
jaredgrubb added a comment.

Update `ClangFormatStyleOptions.rst` as requested by the auto-comment (thanks 
auto-bot!).


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

https://reviews.llvm.org/D150083

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
  llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
  llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1301,6 +1301,7 @@
 "lib/Format/FormatTokenLexer.h",
 "lib/Format/FormatTokenSource.h",
 "lib/Format/Macros.h",
+"lib/Format/ObjCPropertyAttributeOrderFixer.h",
 "lib/Format/QualifierAlignmentFixer.h",
 "lib/Format/UnwrappedLineParser.h",
 ] + glob([
Index: llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
@@ -36,6 +36,7 @@
 "MacroCallReconstructorTest.cpp",
 "MacroExpanderTest.cpp",
 "NamespaceEndCommentsFixerTest.cpp",
+"ObjCPropertyAttributeOrderFixerTest.cpp",
 "QualifierFixerTest.cpp",
 "SortImportsTestJS.cpp",
 "SortImportsTestJava.cpp",
Index: llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
@@ -20,6 +20,7 @@
 "MacroCallReconstructor.cpp",
 "MacroExpander.cpp",
 "NamespaceEndCommentsFixer.cpp",
+"ObjCPropertyAttributeOrderFixer.cpp",
 "QualifierAlignmentFixer.cpp",
 "SortJavaScriptImports.cpp",
 "TokenAnalyzer.cpp",
Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,192 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, Style).annotate(Code);
+  }
+  llvm::SpecificBumpPtrAllocator Allocator;
+  std::vector> Buffers;
+};
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, ParsesStyleOption) {
+  FormatStyle Style = {};
+  Style.Language = FormatStyle::LK_ObjC;
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [class]", ObjCPropertyAttributeOrder,
+  std::vector({"class"}));
+
+  CHECK_PARSE("ObjCPropertyAttributeOrder: [direct, strong]",
+  ObjCPropertyAttributeOrder,
+  std::vector({"direct", "strong"}));
+}
+
+TEST_F(ObjCPropertyAttributeOrderFixerTest, SortsSpecifiedAttributes) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ObjCPropertyAttributeOrder = {"a", "b", "c"};
+
+  verifyFormat("@property() int p;", Style);
+
+  // One: shouldn't move.
+  verifyFormat("@property(a) int p;", Style);
+  

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

Some implementation notes:

- The implementation was modeled after `QualifierAlignmentFixer` and 
`sortCppIncludes`.
- the additions to the `.bazel`/`.gn` build files was done naively based on 
searching for where "QualifierFixerTest" appeared in the repo; I can't really 
test these. It looks right, but it's a guess.
- I considered creating a top-level style similar to 
`QualifierAlignment/QualifierOrder`; that one has a few pre-canned values 
(`Leave, Left, Right, Custom`), where `Custom` opens up the style 
`QualifierOrder`, an array of qualifier names.
  - Pro: we could create a `[Leave, LLVM]` that would set them in the same 
order the `DeclPrinter.cpp` would dump them
  - Pro: users probably would like not having to think about 18 different 
attributes (I did try to document them in groups to make them easier to find)
  - Con: I couldn't find any style guides that provide a "standard order", so I 
don't know any other pre-canned settings that wouldn't just be an opinion of my 
own personal style. If there are, let me know and that might add weight to 
going this route.

I don't like that there are 18 attributes that users _probably_ want to specify 
for the style, so having some shortcut seems helpful. Perhaps I could just 
document it that way so it's easy to copy-paste? I also considered making it 
possible to have an alias so the style would only need to have a handful of 
them to get full coverage (eg, if you specify `assign` in the style, then the 
tool would infer you also mean to imply that the similar attributes `retain, 
strong, copy, weak, unsafe_unretained` would occupy the same "slot" unless you 
also specify them explicitly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150083

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

[PATCH] D150083: [clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb created this revision.
jaredgrubb added reviewers: benhamilton, egorzhdan, owenpan, HazardyKnusperkeks.
jaredgrubb added a project: clang-format.
Herald added projects: All, clang.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, MyDeveloperDay.
Herald added a comment.
jaredgrubb requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does 
not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via 
clang/docs/tools/dump_format_style.py,  please run this to regenerate the .rst

You can validate that the rst is valid by running.

  ./docs/tools/dump_format_style.py
  mkdir -p html
  /usr/bin/sphinx-build -n ./docs ./html


Add a style option to specify the order that property attributes should appear 
in ObjC property declarations (property attributes are things like`nonatomic, 
strong, nullable`).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150083

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/tools/clang-formatted-files.txt
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp
  clang/lib/Format/ObjCPropertyAttributeOrderFixer.h
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
  llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
  llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/clang/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/BUILD.bazel
@@ -1301,6 +1301,7 @@
 "lib/Format/FormatTokenLexer.h",
 "lib/Format/FormatTokenSource.h",
 "lib/Format/Macros.h",
+"lib/Format/ObjCPropertyAttributeOrderFixer.h",
 "lib/Format/QualifierAlignmentFixer.h",
 "lib/Format/UnwrappedLineParser.h",
 ] + glob([
Index: llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn
@@ -36,6 +36,7 @@
 "MacroCallReconstructorTest.cpp",
 "MacroExpanderTest.cpp",
 "NamespaceEndCommentsFixerTest.cpp",
+"ObjCPropertyAttributeOrderFixerTest.cpp",
 "QualifierFixerTest.cpp",
 "SortImportsTestJS.cpp",
 "SortImportsTestJava.cpp",
Index: llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn
@@ -20,6 +20,7 @@
 "MacroCallReconstructor.cpp",
 "MacroExpander.cpp",
 "NamespaceEndCommentsFixer.cpp",
+"ObjCPropertyAttributeOrderFixer.cpp",
 "QualifierAlignmentFixer.cpp",
 "SortJavaScriptImports.cpp",
 "TokenAnalyzer.cpp",
Index: clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp
@@ -0,0 +1,192 @@
+//===- unittest/Format/ObjCPropertyAttributeOrderFixerTest.cpp - unit tests
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../lib/Format/ObjCPropertyAttributeOrderFixer.h"
+#include "FormatTestBase.h"
+#include "TestLexer.h"
+
+#define DEBUG_TYPE "format-objc-property-attribute-order-fixer-test"
+
+namespace clang {
+namespace format {
+namespace test {
+namespace {
+
+#define CHECK_PARSE(TEXT, FIELD, VALUE)\
+  EXPECT_NE(VALUE, Style.FIELD) << "Initial value already the same!";  \
+  EXPECT_EQ(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+#define FAIL_PARSE(TEXT, FIELD, VALUE) \
+  EXPECT_NE(0, parseConfiguration(TEXT, ).value());  \
+  EXPECT_EQ(VALUE, Style.FIELD) << "Unexpected value after parsing!"
+
+class ObjCPropertyAttributeOrderFixerTest : public FormatTestBase {
+protected:
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle  = getLLVMStyle()) {
+return TestLexer(Allocator, Buffers, 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 520195.
jaredgrubb added a comment.

Address review feedback about `code/endcode`. Otherwise the patch is the same 
and I hope ready for merge? I'd love to get a green check :)


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1543,6 +1543,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

I have uploaded patch for all the points you called out. (Sorry for delay; I 
missed the suggestions earlier!)


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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 516211.
jaredgrubb marked 6 inline comments as done.
jaredgrubb added a comment.

Address review comments from @owenpan


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1543,6 +1543,116 @@
   EXPECT_TOKEN(Tokens[13], tok::arrow, TT_Unknown);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCProperty) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("@property(weak) id delegate __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("@property(weak) id delegate 

[PATCH] D146310: [clang-format] Fix dropped 'else' in 398cddf6acec

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

Manuel, if you're happy with the change, do you mind committing it? I don't 
have commit access (at least I've never requested it, so I assume I can't, I've 
never tried)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146310

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

[PATCH] D146310: [clang-format] Fix dropped 'else' in 398cddf6acec

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

Yeh, I considered trying to craft one as courtesy but this seemed like a very 
far edge case and didn't seem really worth it. So glad you agree :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146310

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

[PATCH] D146434: [clang-format] Fix support for ObjC blocks with pointer return types

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:22178
"});");
-  verifyFormat("Block b = ^int *(A *a, B *b) {}");
+  verifyFormat("Block b = ^int *(A *a, B *b) {\n"
+   "};");

MyDeveloperDay wrote:
> I don’t like us changing tests
Normally I would agree, but in this case, this is a bug-fix for the test as 
well. 

If you look at the `verifyFormat` just below this one, its braces are broken 
across separate lines. The inconsistency between these two tests was due to the 
difference in the block's return type (pointer vs non-pointer). 

My patch corrects exactly that inconsistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146434

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

[PATCH] D146434: [clang-format] Fix support for ObjC blocks with pointer return types

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb created this revision.
jaredgrubb added reviewers: djasper, egorzhdan, benhamilton.
jaredgrubb added a project: clang-format.
Herald added a project: All.
jaredgrubb requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The ObjC-block detection code only supports a single token as the return type. 
Add support to detect pointers, too (ObjC has lots of object-pointers).

For example, using `BasedOnStyle: WebKit`, the following is stable output:

  int* p = ^int*(void)
  { //
  return nullptr;
  }
  ();

After the patch:

  int* p = ^int*(void) { //
  return nullptr;
  }();


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146434

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -994,6 +994,20 @@
   verifyFormat("int (^foo[kNumEntries])(char, float);");
   verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
   verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+
+  verifyFormat("int *p = ^int *() { //\n"
+   "  return nullptr;\n"
+   "}();");
+
+  verifyFormat("int * (^p)(void) = ^int *(void) { //\n"
+   "  return nullptr;\n"
+   "};");
+
+  // WebKit forces function braces onto a newline, but blocks should not.
+  Style = getWebKitStyle();
+  verifyFormat("int* p = ^int*() { //\n"
+   "return nullptr;\n"
+   "}();");
 }
 
 TEST_F(FormatTestObjC, ObjCSnippets) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22175,7 +22175,8 @@
"}\n"
"  }\n"
"});");
-  verifyFormat("Block b = ^int *(A *a, B *b) {}");
+  verifyFormat("Block b = ^int *(A *a, B *b) {\n"
+   "};");
   verifyFormat("BOOL (^aaa)(void) = ^BOOL {\n"
"};");
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1773,12 +1773,18 @@
   break;
 case tok::caret:
   nextToken();
+  // Block return type.
   if (FormatTok->Tok.isAnyIdentifier() ||
   FormatTok->isSimpleTypeSpecifier()) {
 nextToken();
+// Return types: pointers are ok too.
+while (FormatTok->is(tok::star))
+  nextToken();
   }
+  // Block argument list.
   if (FormatTok->is(tok::l_paren))
 parseParens();
+  // Block body.
   if (FormatTok->is(tok::l_brace))
 parseChildBlock();
   break;


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -994,6 +994,20 @@
   verifyFormat("int (^foo[kNumEntries])(char, float);");
   verifyFormat("int (^foo[kNumEntries + 10])(char, float);");
   verifyFormat("int (^foo[(kNumEntries + 10)])(char, float);");
+
+  verifyFormat("int *p = ^int *() { //\n"
+   "  return nullptr;\n"
+   "}();");
+
+  verifyFormat("int * (^p)(void) = ^int *(void) { //\n"
+   "  return nullptr;\n"
+   "};");
+
+  // WebKit forces function braces onto a newline, but blocks should not.
+  Style = getWebKitStyle();
+  verifyFormat("int* p = ^int*() { //\n"
+   "return nullptr;\n"
+   "}();");
 }
 
 TEST_F(FormatTestObjC, ObjCSnippets) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22175,7 +22175,8 @@
"}\n"
"  }\n"
"});");
-  verifyFormat("Block b = ^int *(A *a, B *b) {}");
+  verifyFormat("Block b = ^int *(A *a, B *b) {\n"
+   "};");
   verifyFormat("BOOL (^aaa)(void) = ^BOOL {\n"
"};");
 
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -1773,12 +1773,18 @@
   break;
 case tok::caret:
   nextToken();
+  // Block return type.
   if (FormatTok->Tok.isAnyIdentifier() ||
   FormatTok->isSimpleTypeSpecifier()) {
 nextToken();
+// Return types: pointers are ok too.
+while (FormatTok->is(tok::star))
+  nextToken();
   }
+  // Block argument list.
   

[PATCH] D146310: [clang-format] Fix dropped 'else' in 398cddf6acec

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

The difference doesn't appear to affect any unit tests (which is unfortunate), 
but I think you didn't mean to remove this else, based both on the logic of the 
original commit and the format of the patched line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146310

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

[PATCH] D146310: [clang-format] Fix dropped 'else' in 398cddf6acec

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb created this revision.
jaredgrubb added a reviewer: klimek.
jaredgrubb added a project: clang-format.
Herald added a project: All.
jaredgrubb requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

A patch (398cddf6acec 
) appears 
to have inadvertently removed an `else`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146310

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -511,9 +511,9 @@
 ShouldMerge = !Style.BraceWrapping.AfterClass ||
   (NextLine.First->is(tok::r_brace) &&
!Style.BraceWrapping.SplitEmptyRecord);
-  } if(TheLine->InPPDirective ||
-   !TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
-tok::kw_struct)) {
+  } else if (TheLine->InPPDirective ||
+ !TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
+  tok::kw_struct)) {
 // Try to merge a block with left brace unwrapped that wasn't yet
 // covered.
 ShouldMerge = !Style.BraceWrapping.AfterFunction ||


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -511,9 +511,9 @@
 ShouldMerge = !Style.BraceWrapping.AfterClass ||
   (NextLine.First->is(tok::r_brace) &&
!Style.BraceWrapping.SplitEmptyRecord);
-  } if(TheLine->InPPDirective ||
-   !TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
-tok::kw_struct)) {
+  } else if (TheLine->InPPDirective ||
+ !TheLine->First->isOneOf(tok::kw_class, tok::kw_enum,
+  tok::kw_struct)) {
 // Try to merge a block with left brace unwrapped that wasn't yet
 // covered.
 ShouldMerge = !Style.BraceWrapping.AfterFunction ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.



Comment at: clang/unittests/Format/FormatTestObjC.cpp:1619
+  // Reflow after first macro.
+  // FIXME: these should indent but don't.
+  verifyFormat("- (id)init ATTRIBUTE_MACRO(X)\n"

I don't love this FIXME, but I was afraid to add more to this patch, as fixing 
this will require digging into things that have nothing to do with 
`__attribute__` vs `AttributeMacros`.

For example, suffix macros in C/C++ also are broken in the same way with just 
plain `__attribute__`. For example, for `ColumnWidth: 50`:
```
int f(double) __attribute__((overloadable))
__attribute__((overloadable));

int ff(double)
__attribute__((overloadable))
__attribute__((overloadable));
```

I think fixing reflowing of suffix macros is best done in another PR (which I 
can take a stab at!)


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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:5329
+  if (Right.is(tok::l_square) && Right.is(TT_AttributeSquare))
 return !Left.is(TT_AttributeSquare);
 

I broke this `if` into two pieces and restored an original `return true` for 
this part of the `if` that was changed in 2020 (via 
5a4ddbd69db2b0e09398214510501d0e59a0c30b).

The `return !Left...` was added to avoid breaking `[[` apart when reflowing a 
function-arg with C++11-style attribute (eg, `[[unused]]`). As far as I can 
tell, this check _really_ only matters if the code-path is the second-half of 
the original if (`Right.is(tok::l_square)...`). 

I couldn't imagine how there `Right.is(tok::kw___attribute)` needs the 
`Left`-check, so I think that patch is better split to differentiate (even 
though originally it didn't really have any conflicting effect).

Please let me know if my logic here is wrong (or I'm not making sense :) )


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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 504486.
jaredgrubb added a comment.

- Fixed an issue in `TokenAnnotator` about it not breaking between macros 
properly (it was catching in an ObjC selector-check too early)
- Add more ObjC tests, covering method and property declarations too. There are 
still some quirks about reflowing multiple attributes, but those quirks exist 
in C++ too, so I think those are best left for another patch. I added checks 
for existing behavior so that patch can improve the ObjC version too.


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1296,6 +1296,121 @@
   EXPECT_TOKEN(Tokens[9], tok::colon, TT_GenericSelectionColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  // 'TT_FunctionAnnotationRParen' doesn't seem right; fix?
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_FunctionAnnotationRParen);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) @interface Foo");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[0], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[1], tok::l_paren);
+  EXPECT_TOKEN_KIND(Tokens[3], tok::r_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) @interface Foo", Style);
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacrosOnObjCMethodDecl) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("- (id)init __attribute__(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("- (id)init A(X);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  // Note: Don't check token-type as a random token in this position is hard to
+  // reason about.
+  EXPECT_TOKEN_KIND(Tokens[5], tok::identifier);
+  EXPECT_TOKEN_KIND(Tokens[6], tok::l_paren);
+  EXPECT_TOKEN_KIND(Tokens[8], tok::r_paren);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("- (id)init A(X);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_AttributeParen);

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb updated this revision to Diff 502390.
jaredgrubb added a comment.

Create unit-tests for the patch (and remove the proposed non-unit test "test").


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

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1292,6 +1292,34 @@
   EXPECT_TOKEN(Tokens[9], tok::colon, TT_GenericSelectionColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsAttributeMacros) {
+  // '__attribute__' has special handling.
+  auto Tokens = annotate("__attribute__(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::kw___attribute, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+
+  // Generic macro has no special handling in this location.
+  Tokens = annotate("A(X) void Foo(void);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_Unknown);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  // 'TT_FunctionAnnotationRParen' doesn't seem right; fix?
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_FunctionAnnotationRParen);
+
+  // Add a custom AttributeMacro. Test that it has the same behavior.
+  FormatStyle Style = getLLVMStyle();
+  Style.AttributeMacros.push_back("A");
+
+  // An "AttributeMacro" gets annotated like '__attribute__'.
+  Tokens = annotate("A(X) void Foo(void);", Style);
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::identifier, TT_AttributeMacro);
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_AttributeParen);
+  EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_AttributeParen);
+}
+
 TEST_F(TokenAnnotatorTest, UnderstandsVerilogOperators) {
   auto Annotate = [this](llvm::StringRef Code) {
 return annotate(Code, getLLVMStyle(FormatStyle::LK_Verilog));
Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -1489,6 +1489,9 @@
 }
 
 TEST_F(FormatTestObjC, Attributes) {
+  Style.AttributeMacros.push_back("ATTRIBUTE_MACRO");
+
+  // Check '__attribute__' macro directly.
   verifyFormat("__attribute__((objc_subclassing_restricted))\n"
"@interface Foo\n"
"@end");
@@ -1498,6 +1501,41 @@
   verifyFormat("__attribute__((objc_subclassing_restricted))\n"
"@implementation Foo\n"
"@end");
+
+  // Check AttributeMacro gets treated the same, with or without parentheses.
+  verifyFormat("ATTRIBUTE_MACRO\n"
+   "@interface Foo\n"
+   "@end");
+  verifyFormat("ATTRIBUTE_MACRO(X)\n"
+   "@interface Foo\n"
+   "@end");
+
+  // Indenter also needs to understand multiple attribute macros.
+  verifyFormat("ATTRIBUTE_MACRO(X) ATTRIBUTE_MACRO\n"
+   "@interface Foo\n"
+   "@end");
+  verifyFormat("ATTRIBUTE_MACRO ATTRIBUTE_MACRO(X)\n"
+   "@interface Foo\n"
+   "@end");
+
+  Style.ColumnLimit = 30;
+  verifyFormat("ATTRIBUTE_MACRO(X)\n"
+   "ATTRIBUTE_MACRO\n"
+   "@interface Foo\n"
+   "@end");
+  // Note: the following -should- break across multiple lines, but doesn't.
+  // This is added to acknowledge the behavior, but it should be improved.
+  verifyFormat("ATTRIBUTE_MACRO ATTRIBUTE_MACRO(X)\n"
+   "@interface Foo\n"
+   "@end");
+
+  Style.ColumnLimit = 0;
+  verifyFormat("ATTRIBUTE_MACRO(X) ATTRIBUTE_MACRO\n"
+   "@interface Foo\n"
+   "@end");
+  verifyFormat("ATTRIBUTE_MACRO ATTRIBUTE_MACRO(X)\n"
+   "@interface Foo\n"
+   "@end");
 }
 
 } // end namespace
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -369,7 +369,7 @@
 // Infer the role of the l_paren based on the previous token if we haven't
 // detected one yet.
 if (PrevNonComment && OpeningParen.is(TT_Unknown)) {
-  if (PrevNonComment->is(tok::kw___attribute)) {
+  if (PrevNonComment->isOneOf(tok::kw___attribute, TT_AttributeMacro)) {
 OpeningParen.setType(TT_AttributeParen);
   } else if (PrevNonComment->isOneOf(TT_TypenameMacro, tok::kw_decltype,
  tok::kw_typeof,
@@ -4889,8 +4889,10 @@
   }
 
   // Ensure wrapping after __attribute__((XX)) and @interface etc.
-  if (Left.is(TT_AttributeParen) 

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

I wasn't sure about testing (this is my first patch!) and the test-case I did 
was in `clang/test/Format` .. I can look at `clang/unittests/Format` and see 
how to model something like it.

If I do that, would that be in-addition-to or instead-of the test-case I 
included already?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb added a comment.

For background, the current clang-format results in the following 
(`-style="{BasedOnStyle: LLVM, ColumnLimit: 0, AttributeMacros: [MACRO]}`):

  MACRO MACRO(A)
  @interface Foo
  @end
  
  MACRO(A)
  MACRO
  @interface Foo
  @end

This patch improves it (removes the indention and makes both cases have same 
wrapping):

  MACRO MACRO(A)
  @interface Foo
  @end
  
  MACRO(A) MACRO
  @interface Foo
  @end


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145262

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

[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb created this revision.
jaredgrubb added reviewers: HazardyKnusperkeks, djasper, egorzhdan.
jaredgrubb added a project: clang-format.
Herald added a project: All.
jaredgrubb requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I noticed that clang-format was inserting some strange indentation whenever I 
used custom "attribute-like macros"
(things like `FOO_EXTERN` to wrap attribute-visible-default, or macros with 
parentheses like `NS_SWIFT_NAME(...)`).

There are two parts to this fix:

- tokenize the paren after an `AttributeMacro` as a `TT_AttributeParen`
- treat a `AttributeMacro`-without-paren the same as one with a paren (eg, the 
`FOO_EXTERN` case)

I added a new test-case to differentiate a macro that is or is-not a 
`AttributeMacro`; also handled whether the
`ColumnLimit` is set to infinite (0) or a finite value, as part of this patch 
is in `ContinuationIndenter`.

There may be other places that need to handle `TT_AttributeMacro` better, but 
this is at least a step in the right
direction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145262

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/test/Format/objc-definitions.m

Index: clang/test/Format/objc-definitions.m
===
--- /dev/null
+++ clang/test/Format/objc-definitions.m
@@ -0,0 +1,58 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, AttributeMacros: [MACRO]}" \
+// RUN:   | FileCheck -strict-whitespace %s --check-prefixes=CHECK-COMMON,CHECK-ATTRMACRO
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, AttributeMacros: [MACRO], ColumnLimit: 0}" \
+// RUN:   | FileCheck -strict-whitespace %s --check-prefixes=CHECK-COMMON,CHECK-ATTRMACRO
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM}" \
+// RUN:   | FileCheck -strict-whitespace %s --check-prefixes=CHECK-COMMON,CHECK-PLAIN
+// RUN: grep -Ev "// *[A-Z-]+:" %s \
+// RUN:   | clang-format -style="{BasedOnStyle: LLVM, ColumnLimit: 0}" \
+// RUN:   | FileCheck -strict-whitespace %s --check-prefixes=CHECK-COMMON,CHECK-PLAIN-COL0
+
+// CHECK-COMMON: {{^@interface Foo$}}
+// CHECK-COMMON-NEXT: {{^@end$}}
+@interface Foo
+@end
+
+// CHECK-COMMON: {{^MACRO$}}
+// CHECK-COMMON-NEXT: {{^@interface Foo$}}
+// CHECK-COMMON-NEXT: {{^@end$}}
+MACRO
+@interface Foo
+@end
+
+// CHECK-COMMON: {{^MACRO\(A\)$}}
+// CHECK-COMMON-NEXT: {{^@interface Foo$}}
+// CHECK-COMMON-NEXT: {{^@end$}}
+MACRO(A)
+@interface Foo
+@end
+
+// CHECK-ATTRMACRO: {{^MACRO MACRO\(A\)$}}
+// CHECK-ATTRMACRO-NEXT: {{^@interface Foo$}}
+// CHECK-ATTRMACRO-NEXT: {{^@end$}}
+// CHECK-PLAIN: {{^MACRO MACRO\(A\) @interface Foo$}}
+// CHECK-PLAIN-NEXT: {{^@end$}}
+// COM: The leading indentation in the next case is existing behavior but probably not desired.
+// CHECK-PLAIN-COL0: {{^MACRO MACRO\(A\)$}}
+// CHECK-PLAIN-COL0-NEXT: {{^@interface Foo$}}
+// CHECK-PLAIN-COL0-NEXT: {{^@end$}}
+MACRO MACRO(A)
+@interface Foo
+@end
+
+// CHECK-ATTRMACRO: {{^MACRO\(A\) MACRO$}}
+// CHECK-ATTRMACRO-NEXT: {{^@interface Foo$}}
+// CHECK-ATTRMACRO-NEXT: {{^@end$}}
+// CHECK-PLAIN: {{^MACRO\(A\) MACRO @interface Foo$}}
+// CHECK-PLAIN-NEXT: {{^@end$}}
+// COM: The leading indentation in the next case is existing behavior but probably not desired.
+// CHECK-PLAIN-COL0: {{^MACRO\(A\)$}}
+// CHECK-PLAIN-COL0: {{^MACRO$}}
+// CHECK-PLAIN-COL0-NEXT: {{^@interface Foo$}}
+// CHECK-PLAIN-COL0-NEXT: {{^@end$}}
+MACRO(A) MACRO
+@interface Foo
+@end
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -369,7 +369,7 @@
 // Infer the role of the l_paren based on the previous token if we haven't
 // detected one yet.
 if (PrevNonComment && OpeningParen.is(TT_Unknown)) {
-  if (PrevNonComment->is(tok::kw___attribute)) {
+  if (PrevNonComment->isOneOf(tok::kw___attribute, TT_AttributeMacro)) {
 OpeningParen.setType(TT_AttributeParen);
   } else if (PrevNonComment->isOneOf(TT_TypenameMacro, tok::kw_decltype,
  tok::kw_typeof,
@@ -4889,8 +4889,10 @@
   }
 
   // Ensure wrapping after __attribute__((XX)) and @interface etc.
-  if (Left.is(TT_AttributeParen) && Right.is(TT_ObjCDecl))
+  if (Left.isOneOf(TT_AttributeMacro, TT_AttributeParen) &&
+  Right.is(TT_ObjCDecl)) {
 return true;
+  }
 
   if (Left.is(TT_LambdaLBrace)) {
 if (IsFunctionArgument(Left) &&
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -1228,8 +1228,9 @@