[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326191: clang-format: fix formatting of ObjC @synchronized
blocks (authored by Typz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43114?vs=133570&id=136067#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43114
Files:
cfe/trunk/lib/Format/UnwrappedLineParser.cpp
cfe/trunk/unittests/Format/FormatTestObjC.cpp
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1135,6 +1135,18 @@
}
addUnwrappedLine();
return;
+ case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+ // Skip synchronization object
+ parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+ parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
case tok::objc_try:
// This branch isn't strictly necessary (the kw_try case below would
// do this too after the tok::at is parsed above). But be explicit.
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -209,6 +209,24 @@
"a);\n");
}
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ verifyFormat("@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n");
+}
+
TEST_F(FormatTestObjC, FormatObjCInterface) {
verifyFormat("@interface Foo : NSObject {\n"
"@public\n"
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1135,6 +1135,18 @@
}
addUnwrappedLine();
return;
+ case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+ // Skip synchronization object
+ parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+ parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
case tok::objc_try:
// This branch isn't strictly necessary (the kw_try case below would
// do this too after the tok::at is parsed above). But be explicit.
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -209,6 +209,24 @@
"a);\n");
}
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ verifyFormat("@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n");
+}
+
TEST_F(FormatTestObjC, FormatObjCInterface) {
verifyFormat("@interface Foo : NSObject {\n"
"@public\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
Typz marked an inline comment as done.
Typz added inline comments.
Comment at: unittests/Format/FormatTestObjC.cpp:193-198
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
benhamilton wrote:
> Why is the block repeated twice?
not sure why, I just copied the @autoreleasepool test code.
this may be to detect some more 'internal' error, which would lead to any
visible effect on the first statement itself...
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
benhamilton accepted this revision.
benhamilton added a comment.
This revision is now accepted and ready to land.
Just a question on the test.
Comment at: unittests/Format/FormatTestObjC.cpp:193-198
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
Why is the block repeated twice?
Comment at: unittests/Format/FormatTestObjC.cpp:200-207
+ verifyFormat("@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self)\n"
+ "{\n"
+ " f();\n"
Same question.
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
Typz marked 4 inline comments as done.
Typz added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
benhamilton wrote:
> Typz wrote:
> > benhamilton wrote:
> > > Typz wrote:
> > > > Wondering if formatting with this style is appropriate: the
> > > > clang-format doc points in that direction, but it seems to me both
> > > > `@synchronized` and `@autoreleasepool` are more akin to "control
> > > > statements".
> > > >
> > > > For consistency (esp. in ObjC++ code), as a user, I would tend to have
> > > > these blocks indented like control statements while
> > > > interfaces/implementations blocks would be indented like
> > > > classes/structs.
> > > >
> > > > So maybe it would be better to introduce a new BraceWrapping style
> > > > 'AfterObjCSpecialBlock` to control these cases, matching the
> > > > possibilities that are given for control statements vs classes. What do
> > > > you think?
> > > Hmm, I definitely agree with that logic. I don't see them acting as
> > > declarations in any way, they are definitely like control statements.
> > >
> > Ok, i can change this. Two questions though:
> > * Should I do this in this patch, or a separate patch? (won't be a big
> > change in any case, but it can still be seen as 2 separate issues/changes)
> > * Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or
> > simply apply `AfterControlStatement` to these blocks?
> Personally, I feel `AfterControlStatement` applies to these. I'm not an
> expert on this, though, so I will defer to others on the review.
I created another diff to change the field that is used to control brace
wrapping after these blocks: https://reviews.llvm.org/D43232
This patch here should now only relate to the parsing of `@synchronized` blocks.
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
benhamilton added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
Typz wrote:
> benhamilton wrote:
> > Typz wrote:
> > > Wondering if formatting with this style is appropriate: the clang-format
> > > doc points in that direction, but it seems to me both `@synchronized` and
> > > `@autoreleasepool` are more akin to "control statements".
> > >
> > > For consistency (esp. in ObjC++ code), as a user, I would tend to have
> > > these blocks indented like control statements while
> > > interfaces/implementations blocks would be indented like classes/structs.
> > >
> > > So maybe it would be better to introduce a new BraceWrapping style
> > > 'AfterObjCSpecialBlock` to control these cases, matching the
> > > possibilities that are given for control statements vs classes. What do
> > > you think?
> > Hmm, I definitely agree with that logic. I don't see them acting as
> > declarations in any way, they are definitely like control statements.
> >
> Ok, i can change this. Two questions though:
> * Should I do this in this patch, or a separate patch? (won't be a big change
> in any case, but it can still be seen as 2 separate issues/changes)
> * Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or
> simply apply `AfterControlStatement` to these blocks?
Personally, I feel `AfterControlStatement` applies to these. I'm not an expert
on this, though, so I will defer to others on the review.
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
Typz added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
benhamilton wrote:
> Typz wrote:
> > Wondering if formatting with this style is appropriate: the clang-format
> > doc points in that direction, but it seems to me both `@synchronized` and
> > `@autoreleasepool` are more akin to "control statements".
> >
> > For consistency (esp. in ObjC++ code), as a user, I would tend to have
> > these blocks indented like control statements while
> > interfaces/implementations blocks would be indented like classes/structs.
> >
> > So maybe it would be better to introduce a new BraceWrapping style
> > 'AfterObjCSpecialBlock` to control these cases, matching the possibilities
> > that are given for control statements vs classes. What do you think?
> Hmm, I definitely agree with that logic. I don't see them acting as
> declarations in any way, they are definitely like control statements.
>
Ok, i can change this. Two questions though:
* Should I do this in this patch, or a separate patch? (won't be a big change
in any case, but it can still be seen as 2 separate issues/changes)
* Should I introduce a new BraceWrapping flag (`AfterObjCSpecialBlock`), or
simply apply `AfterControlStatement` to these blocks?
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
benhamilton added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
Typz wrote:
> Wondering if formatting with this style is appropriate: the clang-format doc
> points in that direction, but it seems to me both `@synchronized` and
> `@autoreleasepool` are more akin to "control statements".
>
> For consistency (esp. in ObjC++ code), as a user, I would tend to have these
> blocks indented like control statements while interfaces/implementations
> blocks would be indented like classes/structs.
>
> So maybe it would be better to introduce a new BraceWrapping style
> 'AfterObjCSpecialBlock` to control these cases, matching the possibilities
> that are given for control statements vs classes. What do you think?
Hmm, I definitely agree with that logic. I don't see them acting as
declarations in any way, they are definitely like control statements.
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
Typz added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:1130
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
Wondering if formatting with this style is appropriate: the clang-format doc
points in that direction, but it seems to me both `@synchronized` and
`@autoreleasepool` are more akin to "control statements".
For consistency (esp. in ObjC++ code), as a user, I would tend to have these
blocks indented like control statements while interfaces/implementations blocks
would be indented like classes/structs.
So maybe it would be better to introduce a new BraceWrapping style
'AfterObjCSpecialBlock` to control these cases, matching the possibilities that
are given for control statements vs classes. What do you think?
Repository:
rC Clang
https://reviews.llvm.org/D43114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43114: clang-format: fix formatting of ObjC @synchronized blocks
Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.
The blocks used to be formatted using the "default" behavior, and would
thus be mistaken for function calls followed by blocks: this could lead
to unexpected inlining of the block and extra line-break before the
opening brace.
They are now formatted similarly to `@autoreleasepool` blocks, as
expected:
@synchronized(self) {
f();
}
Repository:
rC Clang
https://reviews.llvm.org/D43114
Files:
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTestObjC.cpp
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,24 @@
"}\n");
}
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ verifyFormat("@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n");
+}
+
TEST_F(FormatTestObjC, FormatObjCInterface) {
verifyFormat("@interface Foo : NSObject {\n"
"@public\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1121,6 +1121,18 @@
}
addUnwrappedLine();
return;
+ case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+ // Skip synchronization object
+ parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+ parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
case tok::objc_try:
// This branch isn't strictly necessary (the kw_try case below would
// do this too after the tok::at is parsed above). But be explicit.
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -189,6 +189,24 @@
"}\n");
}
+TEST_F(FormatTestObjC, FormatObjCSynchronized) {
+ verifyFormat("@synchronized(self) {\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self) {\n"
+ " f();\n"
+ "}\n");
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ verifyFormat("@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n"
+ "@synchronized(self)\n"
+ "{\n"
+ " f();\n"
+ "}\n");
+}
+
TEST_F(FormatTestObjC, FormatObjCInterface) {
verifyFormat("@interface Foo : NSObject {\n"
"@public\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1121,6 +1121,18 @@
}
addUnwrappedLine();
return;
+ case tok::objc_synchronized:
+nextToken();
+if (FormatTok->Tok.is(tok::l_paren))
+ // Skip synchronization object
+ parseParens();
+if (FormatTok->Tok.is(tok::l_brace)) {
+ if (Style.BraceWrapping.AfterObjCDeclaration)
+addUnwrappedLine();
+ parseBlock(/*MustBeDeclaration=*/false);
+}
+addUnwrappedLine();
+return;
case tok::objc_try:
// This branch isn't strictly necessary (the kw_try case below would
// do this too after the tok::at is parsed above). But be explicit.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
