[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g created this revision.
guitard0g added reviewers: aaron.ballman, NoQ, vsavchenko.
guitard0g requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The 'escape' attribute indicates that the annotated pointer parameter
may escape the scope of the function. This attribute is meant to be
used for compiler diagnostics/static analysis checks. The attribute
will first be used by the Swift compiler in a new implicit bridging
diagnostic, but may have other non-Swift use-cases for diagnostics.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107026
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/test/AST/ast-dump-attr.cpp
clang/test/AST/ast-dump-attr.m
Index: clang/test/AST/ast-dump-attr.m
===
--- clang/test/AST/ast-dump-attr.m
+++ clang/test/AST/ast-dump-attr.m
@@ -65,4 +65,8 @@
// CHECK-NEXT: | `-NoEscapeAttr
// CHECK-NEXT: |-ParmVarDecl{{.*}} Test14 'int'
// CHECK-NEXT: `-NSConsumesSelfAttr
+-(void)Test15: (int *) [[clang::escape]] Test16;
+// CHECK: ObjCMethodDecl{{.*}} Test15: 'void'
+// CHECK-NEXT: -ParmVarDecl{{.*}} Test16 'int *'
+// CHECK-NEXT: `-EscapeAttr
@end
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -200,6 +200,15 @@
// CHECK-NEXT: NoEscapeAttr
}
+namespace TestEscape {
+ void escapeFunc(int *p0, __attribute__((escape)) int *p1) {}
+ // CHECK: NamespaceDecl{{.*}} TestEscape
+ // CHECK-NEXT: `-FunctionDecl{{.*}} escapeFunc 'void (int *, int *)'
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: EscapeAttr
+}
+
namespace TestSuppress {
[[gsl::suppress("at-namespace")]];
// CHECK: NamespaceDecl{{.*}} TestSuppress
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1553,11 +1553,11 @@
D->addAttr(::new (S.Context) ReturnsNonNullAttr(S.Context, AL));
}
-static void handleNoEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleXEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
if (D->isInvalidDecl())
return;
- // noescape only applies to pointer types.
+ // escape/noescape only applies to pointer types.
QualType T = cast(D)->getType();
if (!S.isValidPointerAttrType(T, /* RefOkay */ true)) {
S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only)
@@ -1565,7 +1565,16 @@
return;
}
- D->addAttr(::new (S.Context) NoEscapeAttr(S.Context, AL));
+ switch (AL.getKind()) {
+ default:
+llvm_unreachable("invalid escape attribute");
+ case ParsedAttr::AT_NoEscape:
+handleSimpleAttribute(S, D, AL);
+return;
+ case ParsedAttr::AT_Escape:
+handleSimpleAttribute(S, D, AL);
+return;
+ }
}
static void handleAssumeAlignedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
@@ -8007,8 +8016,9 @@
case ParsedAttr::AT_ReturnsNonNull:
handleReturnsNonNullAttr(S, D, AL);
break;
+ case ParsedAttr::AT_Escape:
case ParsedAttr::AT_NoEscape:
-handleNoEscapeAttr(S, D, AL);
+handleXEscapeAttr(S, D, AL);
break;
case ParsedAttr::AT_AssumeAligned:
handleAssumeAlignedAttr(S, D, AL);
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -253,6 +253,27 @@
}];
}
+def EscapeDocs : Documentation {
+ let Category = DocCatVariable;
+ let Content = [{
+``escape`` placed on a function parameter of a pointer type is used to indicate
+that the pointer can escape the function. This means that a reference to the object
+the pointer points to that is derived from the parameter value may survive
+after the function returns.
+
+For example:
+
+.. code-block:: c
+
+ int *gp;
+
+ void escapingFunc(__attribute__((escape)) int *p) {
+gp = p;
+ }
+
+ }];
+}
+
def CarriesDependencyDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1949,6 +1949,12 @@
let Documentation = [NoEscapeDocs];
}
+def Escape : Attr {
+ let Spellings = [Clang<"escape">];
+ let Subjects = SubjectList<[ParmVar]>;
+ let Documentation = [EscapeDocs];
+}
+
def AssumeAligned : InheritableAttr {
let Spellings = [GCC<"assume_aligned">];
let Subjects = SubjectList<[ObjCMethod, Function]>;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g added inline comments.
Comment at: clang/include/clang/Basic/AttrDocs.td:260
+``escape`` placed on a function parameter of a pointer type is used to indicate
+that the pointer can escape the function. This means that a reference to the
object
+the pointer points to that is derived from the parameter value may survive
NoQ wrote:
> Are you sure you want "can escape" instead of "will escape"? In the former
> case it's impossible to implement the warning without false positives ("well,
> it says it may escape, but I know for sure that if I pass these other
> arguments then it won't escape"). In the latter case, of course, a lot of
> places where the value escapes conditionally won't be able to wear the
> attribute. Do I understand correctly that you want to proceed with the more
> aggressive diagnostic?
Hmm that's an interesting question. Initially my thought was that any
possibility of escaping should eliminate the option of passing in a temporary
pointer, which I think is reasonable in Swift's implicit bridging use-case. In
terms of the API contract, I was thinking that any API author who annotates
their function parameter with 'escaping' is effectively saying you shouldn't
pass in a pointer that you're not comfortable escaping. I think it may be a
little restrictive to say that the pointer will always escape for certain, but
I do think that anybody using a function that takes an escaping parameter
should treat this as if it were always escaping (if that makes sense). I
suppose my question then would be, do you think it's better to say "will
escape" if that's what the client calling into an API should expect (even
though the pointer might not always escape in the implementation)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107026/new/
https://reviews.llvm.org/D107026
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g updated this revision to Diff 362643.
guitard0g added a comment.
Herald added a subscriber: jdoerfert.
Change Escape/NoEscape to use InheritableAttr and update an attribute test to
fix test failure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107026/new/
https://reviews.llvm.org/D107026
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/test/AST/ast-dump-attr.cpp
clang/test/AST/ast-dump-attr.m
clang/test/Misc/pragma-attribute-supported-attributes-list.test
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -61,6 +61,7 @@
// CHECK-NEXT: EnforceTCB (SubjectMatchRule_function)
// CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function)
// CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: Escape (SubjectMatchRule_variable_is_parameter)
// CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
// CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
// CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
Index: clang/test/AST/ast-dump-attr.m
===
--- clang/test/AST/ast-dump-attr.m
+++ clang/test/AST/ast-dump-attr.m
@@ -65,4 +65,8 @@
// CHECK-NEXT: | `-NoEscapeAttr
// CHECK-NEXT: |-ParmVarDecl{{.*}} Test14 'int'
// CHECK-NEXT: `-NSConsumesSelfAttr
+-(void)Test15: (int *) [[clang::escape]] Test16;
+// CHECK: ObjCMethodDecl{{.*}} Test15: 'void'
+// CHECK-NEXT: -ParmVarDecl{{.*}} Test16 'int *'
+// CHECK-NEXT: `-EscapeAttr
@end
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -200,6 +200,15 @@
// CHECK-NEXT: NoEscapeAttr
}
+namespace TestEscape {
+ void escapeFunc(int *p0, __attribute__((escape)) int *p1) {}
+ // CHECK: NamespaceDecl{{.*}} TestEscape
+ // CHECK-NEXT: `-FunctionDecl{{.*}} escapeFunc 'void (int *, int *)'
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: EscapeAttr
+}
+
namespace TestSuppress {
[[gsl::suppress("at-namespace")]];
// CHECK: NamespaceDecl{{.*}} TestSuppress
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -1553,11 +1553,11 @@
D->addAttr(::new (S.Context) ReturnsNonNullAttr(S.Context, AL));
}
-static void handleNoEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+static void handleXEscapeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
if (D->isInvalidDecl())
return;
- // noescape only applies to pointer types.
+ // escape/noescape only applies to pointer types.
QualType T = cast(D)->getType();
if (!S.isValidPointerAttrType(T, /* RefOkay */ true)) {
S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only)
@@ -1565,7 +1565,16 @@
return;
}
- D->addAttr(::new (S.Context) NoEscapeAttr(S.Context, AL));
+ switch (AL.getKind()) {
+ default:
+llvm_unreachable("invalid escape attribute");
+ case ParsedAttr::AT_NoEscape:
+handleSimpleAttribute(S, D, AL);
+return;
+ case ParsedAttr::AT_Escape:
+handleSimpleAttribute(S, D, AL);
+return;
+ }
}
static void handleAssumeAlignedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
@@ -8007,8 +8016,9 @@
case ParsedAttr::AT_ReturnsNonNull:
handleReturnsNonNullAttr(S, D, AL);
break;
+ case ParsedAttr::AT_Escape:
case ParsedAttr::AT_NoEscape:
-handleNoEscapeAttr(S, D, AL);
+handleXEscapeAttr(S, D, AL);
break;
case ParsedAttr::AT_AssumeAligned:
handleAssumeAlignedAttr(S, D, AL);
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -253,6 +253,27 @@
}];
}
+def EscapeDocs : Documentation {
+ let Category = DocCatVariable;
+ let Content = [{
+``escape`` placed on a function parameter of a pointer type is used to indicate
+that the pointer can escape the function. This means that a reference to the object
+the pointer points to that is derived from the parameter value may survive
+
[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g marked an inline comment as done.
guitard0g added inline comments.
Comment at: clang/include/clang/Basic/Attr.td:1952
+def Escape : Attr {
+ let Spellings = [Clang<"escape">];
NoQ wrote:
> Shouldn't both this attribute and the one above be `InheritableAttr`?
>
> Comments in this file say:
> ```
> /// An inheritable attribute is inherited by later redeclarations.
> ```
> Which is arguably exactly what we want? Like, it should be sufficient to put
> the attribute into the header, there's no need to duplicate it in the
> implementation?
Oh! Good point, I totally agree.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107026/new/
https://reviews.llvm.org/D107026
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g updated this revision to Diff 362849.
guitard0g marked an inline comment as done.
guitard0g added a comment.
Add tests for diagnostics of incorrect usage. Diagnose when escape and noescape
are used on the same parameter.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107026/new/
https://reviews.llvm.org/D107026
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/test/AST/ast-dump-attr.cpp
clang/test/AST/ast-dump-attr.m
clang/test/Misc/pragma-attribute-supported-attributes-list.test
clang/test/SemaObjCXX/escape.mm
Index: clang/test/SemaObjCXX/escape.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/escape.mm
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++1z %s
+
+struct S {};
+
+template
+void escapeFunc0(__attribute__((noescape)) T); // expected-warning {{'noescape' attribute only applies to pointer arguments}}
+template
+void escapeFunc1(__attribute__((escape)) const T &);
+template
+void escapeFunc2(__attribute__((escape)) T &&);
+
+void escapeFunc3(__attribute__((escape)) int *);
+void escapeFunc4(__attribute__((escape)) void *);
+void escapeFunc5(__attribute__((escape)) int **);
+
+void invalidFunc0(int __attribute__((escape))); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+void invalidFunc1(int __attribute__((escape(0; // expected-error {{'escape' attribute takes no arguments}}
+void invalidFunc2(__attribute__((escape)) int (S::*Ty)); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+void invalidFunc3(__attribute__((escape)) void (S::*Ty)()); // expected-warning {{'escape' attribute only applies to pointer arguments}}
+
+void invalidFunc4(__attribute__((escape)) __attribute__((noescape)) int *); // expected-error {{'noescape' and 'escape' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+void invalidFunc5(__attribute__((noescape)) __attribute__((escape)) int *); // expected-error {{'escape' and 'noescape' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+
+int __attribute__((escape)) g; // expected-warning {{'escape' attribute only applies to parameters}}
+int * __attribute__((escape)) h; // expected-warning {{'escape' attribute only applies to parameters}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -61,6 +61,7 @@
// CHECK-NEXT: EnforceTCB (SubjectMatchRule_function)
// CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function)
// CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: Escape (SubjectMatchRule_variable_is_parameter)
// CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
// CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
// CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
Index: clang/test/AST/ast-dump-attr.m
===
--- clang/test/AST/ast-dump-attr.m
+++ clang/test/AST/ast-dump-attr.m
@@ -65,4 +65,8 @@
// CHECK-NEXT: | `-NoEscapeAttr
// CHECK-NEXT: |-ParmVarDecl{{.*}} Test14 'int'
// CHECK-NEXT: `-NSConsumesSelfAttr
+-(void)Test15: (int *) [[clang::escape]] Test16;
+// CHECK: ObjCMethodDecl{{.*}} Test15: 'void'
+// CHECK-NEXT: -ParmVarDecl{{.*}} Test16 'int *'
+// CHECK-NEXT: `-EscapeAttr
@end
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -200,6 +200,15 @@
// CHECK-NEXT: NoEscapeAttr
}
+namespace TestEscape {
+ void escapeFunc(int *p0, __attribute__((escape)) int *p1) {}
+ // CHECK: NamespaceDecl{{.*}} TestEscape
+ // CHECK-NEXT: `-FunctionDecl{{.*}} escapeFunc 'void (int *, int *)'
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: ParmVarDecl
+ // CHECK-NEXT: EscapeAttr
+}
+
namespace TestSuppress {
[[gsl::suppress("at-namespace")]];
// CHECK: NamespaceDecl{{.*}} TestSuppress
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAt
[PATCH] D107026: [Clang] Add support for attribute 'escape'
guitard0g added a comment. @vsavchenko > I see that the "applies to pointer arguments only" warning is not tested for > `noescape`, but I still find it to be a good practice to write a test with a > bunch of cases with attributes applied in wrong places. Updated with some tests for this! > Additionally, I believe that `escape` and `noescape` convey the same > information, and it is totally okay to have two attributes because the user > has to have a choice to say either "I do escape it" or "I definitely do not > escape it" when the default is unspecified at all. But I do think that we > should add a check disallowing both of these attributes to be used at the > same time for the same parameter, since they directly contradict one another. Ah that's a great point, most recent update makes these two mutually exclusive. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107026/new/ https://reviews.llvm.org/D107026 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g created this revision.
guitard0g requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Currently constructor initializer lists sometimes format incorrectly
when there is a preprocessor directive in the middle of the list.
This patch fixes the issue when parsing the initilizer list by
ignoring the preprocessor directive when checking if a block is
part of an initializer list.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,38 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
++ReadTokens;
} while (NextTok->is(tok::comment));
+// Skip over preprocessor lines, otherwise we may not
+// properly diagnose the block as a braced intializer
+// if the comma separator appears after the pp directive.
+if (NextTok->is(tok::hash)) {
+ ScopedMacroState MacroState(*Line, Tokens, NextTok);
+ do {
+NextTok = Tokens->getNextToken();
+++ReadTokens;
+ } while (NextTok->isNot(tok::eof));
+}
+
switch (Tok->Tok.getKind()) {
case tok::l_brace:
if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,38 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
++ReadTokens;
} while (NextTok->is(tok::comment));
+// Skip over preprocessor lines, otherwise we may not
+// properly diagnose the block as a braced intializer
+// if the comma separator appears after the pp directive.
+if (NextTok->is(tok::hash)) {
+ ScopedMacroState MacroState(*Line, Tokens, NextTok);
+ do {
+
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g added a comment.
@MyDeveloperDay This is an issue a coworker pointed out to me. Previously this
code:
Foo::Foo(int x)
: _x { x }
#if DEBUG
, _y { 0 }
#endif
{
}
would format into the following:
Foo::Foo(int x)
: _x
{
x
}
#if DEBUG
, _y
{
0
}
#endif
{
}
whereas this code without the preprocessor directive:
Foo::Foo(int x)
: _x { x }
, _y { 0 }
{
}
Is properly formatted with no changes made by clang-format.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g updated this revision to Diff 373158.
guitard0g added a comment.
Removing useless line from test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
++ReadTokens;
} while (NextTok->is(tok::comment));
+// Skip over preprocessor lines, otherwise we may not
+// properly diagnose the block as a braced intializer
+// if the comma separator appears after the pp directive.
+if (NextTok->is(tok::hash)) {
+ ScopedMacroState MacroState(*Line, Tokens, NextTok);
+ do {
+NextTok = Tokens->getNextToken();
+++ReadTokens;
+ } while (NextTok->isNot(tok::eof));
+}
+
switch (Tok->Tok.getKind()) {
case tok::l_brace:
if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
++ReadTokens;
} while (NextTok->is(tok::comment));
+// Skip over preprocessor lines, otherwise we may not
+// properly diagnose the block as a braced intializer
+// if the comma separator appears after the pp directive.
+if (NextTok->is(tok::hash)) {
+ ScopedMacroState MacroState(*Line, Tokens, NextTok);
+ do {
+NextTok = Tokens->getNextToken();
+++ReadTokens;
+ } while (NextTok->isNot(tok::eof));
+}
+
switch (Tok->Tok.getKind()) {
case tok::l_brace:
if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g updated this revision to Diff 373167.
guitard0g added a comment.
Fix failing tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
if (Style.Language == FormatStyle::LK_Proto) {
ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
} else {
+ // Skip NextTok over preprocessor lines, otherwise we may not
+ // properly diagnose the block as a braced intializer
+ // if the comma separator appears after the pp directive.
+ if (NextTok->is(tok::hash)) {
+ScopedMacroState MacroState(*Line, Tokens, NextTok);
+do {
+ NextTok = Tokens->getNextToken();
+ ++ReadTokens;
+} while (NextTok->isNot(tok::eof));
+ }
+
// Using OriginalColumn to distinguish between ObjC methods and
// binary operators is a bit hacky.
bool NextIsObjCMethod = NextTok->isOneOf(tok::plus, tok::minus) &&
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
if (Style.Language == FormatStyle::LK_Proto) {
ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
} else {
+ // Skip NextTok over preprocessor lines, otherwise we may not
+ // properly diagnose the block as a braced intializer
+ // if the comma separator appears after the pp directive.
+ if (NextTok->is(tok::hash)) {
+ScopedMacroState MacroState(*Line, Tokens, NextTok);
+do {
+ NextTok = Tokens->getNextToken();
+
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g updated this revision to Diff 373351.
guitard0g added a comment.
Add test case suggestions from reviewers.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ ", b{b} {}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if WINDOWS\n"
+ "#if DEBUG\n"
+ ", b{0}\n"
+ "#else\n"
+ ", b{1}\n"
+ "#endif\n"
+ "#else\n"
+ "#if DEBUG\n"
+ ", b{2}\n"
+ "#else\n"
+ ", b{3}\n"
+ "#endif\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
if (Style.Language == FormatStyle::LK_Proto) {
ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
} else {
+ // Skip NextTok over preprocessor lines, otherwise we may not
+ // properly diagnose the block as a braced intializer
+ // if the comma separator appears after the pp directive.
+ if (NextTok->is(tok::hash)) {
+ScopedMacroState MacroState(*Line, Tokens, NextTok);
+do {
+ NextTok = Tokens->getNextToken();
+ ++ReadTokens;
+} while (NextTok->isNot(tok::eof));
+ }
+
// Using OriginalColumn to distinguish between ObjC methods and
// binary operators is a bit hacky.
bool NextIsObjCMethod = NextTok->isOneOf(tok::plus, tok::minus) &&
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ ", b{b} {}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeCl
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g added a comment.
When looking at test case suggestions, I happened upon another problem that
occurs when handling preprocessor directives. The following code is properly
formatted (following llvm style, with ColumnLimit=0):
SomeClass::SomeClass()
: a{a},
b{b} {}
However this code:
Foo::Foo()
: x{x},
#if DEBUG
y{y},
#endif
z{z} {}
Is transformed by clang-format (under the same style guidelines) into this:
Foo::Foo()
: x{x},
#if DEBUG
y{y},
#endif
z{z} {
}
It seems there is some wonkiness with how it handles preprocessor statements
here. I can address this in a another patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g updated this revision to Diff 373366.
guitard0g added a comment.
Fix failing tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ ", b{b} {}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if WINDOWS\n"
+ "#if DEBUG\n"
+ ", b{0}\n"
+ "#else\n"
+ ", b{1}\n"
+ "#endif\n"
+ "#else\n"
+ "#if DEBUG\n"
+ ", b{2}\n"
+ "#else\n"
+ ", b{3}\n"
+ "#endif\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
if (Style.Language == FormatStyle::LK_Proto) {
ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
} else {
+ // Skip NextTok over preprocessor lines, otherwise we may not
+ // properly diagnose the block as a braced intializer
+ // if the comma separator appears after the pp directive.
+ while (NextTok->is(tok::hash)) {
+ScopedMacroState MacroState(*Line, Tokens, NextTok);
+do {
+ NextTok = Tokens->getNextToken();
+ ++ReadTokens;
+} while (NextTok->isNot(tok::eof));
+ }
+
// Using OriginalColumn to distinguish between ObjC methods and
// binary operators is a bit hacky.
bool NextIsObjCMethod = NextTok->isOneOf(tok::plus, tok::minus) &&
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ ", b{b} {}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g updated this revision to Diff 373977.
guitard0g added a comment.
Add test case for space between directives.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109951/new/
https://reviews.llvm.org/D109951
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,78 @@
Style);
}
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+ FormatStyle Style = getLLVMStyle();
+ Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ ", b{b} {}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if CONDITION\n"
+ ", b{b}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 2;
+ verifyFormat("SomeClass::Constructor()\n"
+ "#if CONDITION\n"
+ " : a{a}\n"
+ "#endif\n"
+ " , b{b}\n"
+ " , c{c} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 0;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#ifdef CONDITION\n"
+ ", b{b}\n"
+ "#else\n"
+ ", c{c}\n"
+ "#endif\n"
+ ", d{d} {\n}",
+ Style);
+ Style.ConstructorInitializerIndentWidth = 4;
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if WINDOWS\n"
+ "#if DEBUG\n"
+ ", b{0}\n"
+ "#else\n"
+ ", b{1}\n"
+ "#endif\n"
+ "#else\n"
+ "#if DEBUG\n"
+ ", b{2}\n"
+ "#else\n"
+ ", b{3}\n"
+ "#endif\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+ verifyFormat("SomeClass::Constructor()\n"
+ ": a{a}\n"
+ "#if WINDOWS\n"
+ ", b{0}\n"
+ "#if DEBUG\n"
+ ", c{0}\n"
+ "#else\n"
+ ", c{1}\n"
+ "#endif\n"
+ "#else\n"
+ "#if DEBUG\n"
+ ", c{2}\n"
+ "#else\n"
+ ", c{3}\n"
+ "#endif\n"
+ ", b{1}\n"
+ "#endif\n"
+ "{\n}",
+ Style);
+}
+
TEST_F(FormatTest, Destructors) {
verifyFormat("void F(int &i) { i.~int(); }");
verifyFormat("void F(int &i) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
if (Style.Language == FormatStyle::LK_Proto) {
ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
} else {
+ // Skip NextTok over preprocessor lines, otherwise we may not
+ // properly diagnose the block as a braced intializer
+ // if the comma separator appears after the pp directive.
+ while (NextTok->is(tok::hash)) {
+ScopedMacroState MacroState(*Line, Tokens, NextTok);
+do {
+ NextTok = Tokens->getNextToken();
+ ++ReadTokens;
+} while (NextTok->isNot(tok::eof));
+ }
+
// Using OriginalColumn to distinguish between ObjC methods and
// binary operators is a bit hacky.
bool NextIsObjCMethod = NextTok->isOneOf(tok::plus, tok::minus) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives
guitard0g added a comment. @MyDeveloperDay @HazardyKnusperkeks I don't have commit access, could one of you commit this for me? Thanks so much for your review! Name: Josh Learn Email: joshua_le...@apple.com (The test failures seem to be unrelated, but I'm fine waiting until they start passing again if necessary) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109951/new/ https://reviews.llvm.org/D109951 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g created this revision.
guitard0g requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D113455
Files:
clang/lib/CodeGen/CGObjCMac.cpp
clang/test/CodeGenObjC/category-class-empty.m
clang/test/CodeGenObjC/non-lazy-classes.m
Index: clang/test/CodeGenObjC/non-lazy-classes.m
===
--- clang/test/CodeGenObjC/non-lazy-classes.m
+++ clang/test/CodeGenObjC/non-lazy-classes.m
@@ -42,4 +42,7 @@
@implementation E @end
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end
Index: clang/test/CodeGenObjC/category-class-empty.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/category-class-empty.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"_OBJC_$_CATEGORY_A_$_foo" = internal global %struct._category_t
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -6659,6 +6659,7 @@
values.add(GetClassGlobal(Interface, /*metaclass*/ false, NotForDefinition));
std::string listName =
(Interface->getObjCRuntimeNameAsString() + "_$_" + OCD->getName()).str();
+ bool isEmptyCategory = true; // Keep track of whether we have actual metadata to emit.
SmallVector instanceMethods;
SmallVector classMethods;
@@ -6672,10 +6673,13 @@
}
}
- values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-instanceMethods));
- values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-classMethods));
+ auto instanceMethodList = emitMethodList(listName, MethodListType::CategoryInstanceMethods,
+ instanceMethods);
+ auto classMethodList = emitMethodList(listName, MethodListType::CategoryClassMethods,
+classMethods);
+ values.add(instanceMethodList);
+ values.add(classMethodList);
+ isEmptyCategory &= instanceMethodList->isNullValue() && classMethodList->isNullValue();
const ObjCCategoryDecl *Category =
Interface->FindCategoryDeclaration(OCD->getIdentifier());
@@ -6683,15 +6687,20 @@
SmallString<256> ExtName;
llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
<< OCD->getName();
-values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
- + Interface->getObjCRuntimeNameAsString() + "_$_"
- + Category->getName(),
-Category->protocol_begin(),
-Category->protocol_end()));
-values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, false));
-values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, true));
+auto protocolList = EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
+ + Interface->getObjCRuntimeNameAsString() + "_$_"
+ + Category->getName(),
+ Category->protocol_begin(),
+ Category->protocol_end());
+auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, false);
+auto classPropertyList = EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, true);
+values.add(protocolList);
+values.add(propertyList);
+values.add(classPropertyList);
+isEmptyCategory &= protocolList->isNullValue() && propertyList->isNullValue() &&
+ classPropertyList->isNullValue();
} else {
values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
@@ -6703,15 +6712,18 @@
llvm::GlobalVariable *GCATV =
finishAndCreateGlobal(values, ExtCatName.str(), CGM);
- CGM.addCompilerUsedGlobal(GCATV);
- if (Interface->hasAttr())
-DefinedStubCategories.push_back(GCATV);
- else
-DefinedCategories.push_back(GCATV);
- // Determine if this category is also "non-lazy".
- if (ImplementationIsNonLazy(OCD))
-DefinedNonLazyCategories.push_back(GCATV);
+ if (!isEmptyCategor
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g updated this revision to Diff 385870.
guitard0g added a comment.
Fixed code formatting.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113455/new/
https://reviews.llvm.org/D113455
Files:
clang/lib/CodeGen/CGObjCMac.cpp
clang/test/CodeGenObjC/category-class-empty.m
clang/test/CodeGenObjC/non-lazy-classes.m
Index: clang/test/CodeGenObjC/non-lazy-classes.m
===
--- clang/test/CodeGenObjC/non-lazy-classes.m
+++ clang/test/CodeGenObjC/non-lazy-classes.m
@@ -42,4 +42,7 @@
@implementation E @end
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end
Index: clang/test/CodeGenObjC/category-class-empty.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/category-class-empty.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"_OBJC_$_CATEGORY_A_$_foo" = internal global %struct._category_t
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -6659,6 +6659,8 @@
values.add(GetClassGlobal(Interface, /*metaclass*/ false, NotForDefinition));
std::string listName =
(Interface->getObjCRuntimeNameAsString() + "_$_" + OCD->getName()).str();
+ // Keep track of whether we have actual metadata to emit.
+ bool isEmptyCategory = true;
SmallVector instanceMethods;
SmallVector classMethods;
@@ -6672,46 +6674,61 @@
}
}
- values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-instanceMethods));
- values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-classMethods));
+ auto instanceMethodList = emitMethodList(
+ listName, MethodListType::CategoryInstanceMethods, instanceMethods);
+ auto classMethodList = emitMethodList(
+ listName, MethodListType::CategoryClassMethods, classMethods);
+ values.add(instanceMethodList);
+ values.add(classMethodList);
+ isEmptyCategory &=
+ instanceMethodList->isNullValue() && classMethodList->isNullValue();
const ObjCCategoryDecl *Category =
-Interface->FindCategoryDeclaration(OCD->getIdentifier());
+ Interface->FindCategoryDeclaration(OCD->getIdentifier());
if (Category) {
SmallString<256> ExtName;
-llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
- << OCD->getName();
-values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
- + Interface->getObjCRuntimeNameAsString() + "_$_"
- + Category->getName(),
-Category->protocol_begin(),
-Category->protocol_end()));
-values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, false));
-values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, true));
+llvm::raw_svector_ostream(ExtName)
+<< Interface->getObjCRuntimeNameAsString() << "_$_" << OCD->getName();
+auto protocolList =
+EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_" +
+ Interface->getObjCRuntimeNameAsString() + "_$_" +
+ Category->getName(),
+ Category->protocol_begin(), Category->protocol_end());
+auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, false);
+auto classPropertyList =
+EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(), OCD,
+ Category, ObjCTypes, true);
+values.add(protocolList);
+values.add(propertyList);
+values.add(classPropertyList);
+isEmptyCategory &= protocolList->isNullValue() &&
+ propertyList->isNullValue() &&
+ classPropertyList->isNullValue();
} else {
values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
}
- unsigned Size = CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
+ unsigned Size =
+ CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
values.addInt(ObjCTypes.IntTy, Size);
llvm::GlobalVariable *GCATV =
finishAndCreateGlobal(values, ExtCatName.s
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g updated this revision to Diff 386943.
guitard0g added a comment.
Address comments and fix fragile test to use -O0 and -disable-llvm-passes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113455/new/
https://reviews.llvm.org/D113455
Files:
clang/lib/CodeGen/CGObjCMac.cpp
clang/test/CodeGenObjC/category-class-empty.m
clang/test/CodeGenObjC/non-lazy-classes.m
Index: clang/test/CodeGenObjC/non-lazy-classes.m
===
--- clang/test/CodeGenObjC/non-lazy-classes.m
+++ clang/test/CodeGenObjC/non-lazy-classes.m
@@ -42,4 +42,7 @@
@implementation E @end
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end
Index: clang/test/CodeGenObjC/category-class-empty.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/category-class-empty.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O0 -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"OBJC_LABEL_CATEGORY_$" = private global [1 x i8*] [i8* bitcast (%struct._category_t* @"_OBJC_$_CATEGORY_A_$_foo"
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -6672,46 +6672,62 @@
}
}
- values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-instanceMethods));
- values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-classMethods));
+ auto instanceMethodList = emitMethodList(
+ listName, MethodListType::CategoryInstanceMethods, instanceMethods);
+ auto classMethodList = emitMethodList(
+ listName, MethodListType::CategoryClassMethods, classMethods);
+ values.add(instanceMethodList);
+ values.add(classMethodList);
+ // Keep track of whether we have actual metadata to emit.
+ bool isEmptyCategory =
+ instanceMethodList->isNullValue() && classMethodList->isNullValue();
const ObjCCategoryDecl *Category =
-Interface->FindCategoryDeclaration(OCD->getIdentifier());
+ Interface->FindCategoryDeclaration(OCD->getIdentifier());
if (Category) {
SmallString<256> ExtName;
-llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
- << OCD->getName();
-values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
- + Interface->getObjCRuntimeNameAsString() + "_$_"
- + Category->getName(),
-Category->protocol_begin(),
-Category->protocol_end()));
-values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, false));
-values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, true));
+llvm::raw_svector_ostream(ExtName)
+<< Interface->getObjCRuntimeNameAsString() << "_$_" << OCD->getName();
+auto protocolList =
+EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_" +
+ Interface->getObjCRuntimeNameAsString() + "_$_" +
+ Category->getName(),
+ Category->protocol_begin(), Category->protocol_end());
+auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, false);
+auto classPropertyList =
+EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(), OCD,
+ Category, ObjCTypes, true);
+values.add(protocolList);
+values.add(propertyList);
+values.add(classPropertyList);
+isEmptyCategory &= protocolList->isNullValue() &&
+ propertyList->isNullValue() &&
+ classPropertyList->isNullValue();
} else {
values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
}
- unsigned Size = CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
+ unsigned Size =
+ CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
values.addInt(ObjCTypes.IntTy, Size);
llvm::GlobalVariable *GCATV =
finishAndCreateGlobal(values, ExtCatName.str(), CGM);
- CGM.addCompilerUsedGlobal(GCATV);
- if (Interface->hasAttr())
-DefinedStubCategories.push_back(GCATV);
- else
-DefinedCategories.push_back(GCATV);
- //
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g updated this revision to Diff 386947.
guitard0g added a comment.
Abandon builder earlier before creating global variable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113455/new/
https://reviews.llvm.org/D113455
Files:
clang/lib/CodeGen/CGObjCMac.cpp
clang/test/CodeGenObjC/category-class-empty.m
clang/test/CodeGenObjC/non-lazy-classes.m
Index: clang/test/CodeGenObjC/non-lazy-classes.m
===
--- clang/test/CodeGenObjC/non-lazy-classes.m
+++ clang/test/CodeGenObjC/non-lazy-classes.m
@@ -42,4 +42,7 @@
@implementation E @end
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end
Index: clang/test/CodeGenObjC/category-class-empty.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/category-class-empty.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O0 -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"OBJC_LABEL_CATEGORY_$" = private global [1 x i8*] [i8* bitcast (%struct._category_t* @"_OBJC_$_CATEGORY_A_$_foo"
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -6672,37 +6672,58 @@
}
}
- values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-instanceMethods));
- values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-classMethods));
+ auto instanceMethodList = emitMethodList(
+ listName, MethodListType::CategoryInstanceMethods, instanceMethods);
+ auto classMethodList = emitMethodList(
+ listName, MethodListType::CategoryClassMethods, classMethods);
+ values.add(instanceMethodList);
+ values.add(classMethodList);
+ // Keep track of whether we have actual metadata to emit.
+ bool isEmptyCategory =
+ instanceMethodList->isNullValue() && classMethodList->isNullValue();
const ObjCCategoryDecl *Category =
-Interface->FindCategoryDeclaration(OCD->getIdentifier());
+ Interface->FindCategoryDeclaration(OCD->getIdentifier());
if (Category) {
SmallString<256> ExtName;
-llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
- << OCD->getName();
-values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
- + Interface->getObjCRuntimeNameAsString() + "_$_"
- + Category->getName(),
-Category->protocol_begin(),
-Category->protocol_end()));
-values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, false));
-values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, true));
+llvm::raw_svector_ostream(ExtName)
+<< Interface->getObjCRuntimeNameAsString() << "_$_" << OCD->getName();
+auto protocolList =
+EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_" +
+ Interface->getObjCRuntimeNameAsString() + "_$_" +
+ Category->getName(),
+ Category->protocol_begin(), Category->protocol_end());
+auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, false);
+auto classPropertyList =
+EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(), OCD,
+ Category, ObjCTypes, true);
+values.add(protocolList);
+values.add(propertyList);
+values.add(classPropertyList);
+isEmptyCategory &= protocolList->isNullValue() &&
+ propertyList->isNullValue() &&
+ classPropertyList->isNullValue();
} else {
values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
}
- unsigned Size = CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
+ if (isEmptyCategory) {
+// Empty category, don't emit any metadata.
+values.abandon();
+MethodDefinitions.clear();
+return;
+ }
+
+ unsigned Size =
+ CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
values.addInt(ObjCTypes.IntTy, Size);
llvm::GlobalVariable *GCATV =
finishAndCreateGlobal(values, ExtCatName.str(), CGM);
+
CGM.addCompilerUsedGlobal(GCAT
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g marked 6 inline comments as done.
guitard0g added inline comments.
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6683
+ values.add(classMethodList);
+ isEmptyCategory &=
+ instanceMethodList->isNullValue() && classMethodList->isNullValue();
zoecarver wrote:
> Does this really need to be an `&=`? Is there any case where
> `isEmptyCategory` isn't `true` here?
Good point, fixed.
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6713
}
+ unsigned Size =
ahatanak wrote:
> Is it not possible to check whether the category is empty here? If it's
> empty, you can avoid creating the global variable and abandon the builder.
Good point, done.
Comment at: clang/test/CodeGenObjC/category-class-empty.m:1
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s |
FileCheck %s
+// PR7431
jkorous wrote:
> zoecarver wrote:
> > Is `-O3` needed here?
> I would even think `-O0` is better and consider also `-disable-llvm-passes`.
> Ultimately the goal is to make sure frontend codegen doesn't emit the
> metadata so the less processing of the IR in the backend the more sensitive
> the test will be.
Good catch, changed to -O0 with -disable-llvm-passes.
Comment at: clang/test/CodeGenObjC/category-class-empty.m:10
+@interface A(foo)
+- (void)foo_myStuff;
+@end
jkorous wrote:
> I assume all the cases when we want to emit the metadata (at least one
> instance method, at least one class method, ...) are covered by other tests,
> right?
> If there's a case that's missing we should add a test for it.
Yeah a fair number of tests would break if we started removing categories with
anything inside of them.
Comment at: clang/test/CodeGenObjC/non-lazy-classes.m:45
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
zoecarver wrote:
> This is to prevent another test from failing? Makes sense.
Yeah this is the one case where the IRGen check was looking for the category
even though it's empty. Should be a harmless change.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113455/new/
https://reviews.llvm.org/D113455
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g updated this revision to Diff 386949.
guitard0g marked 5 inline comments as done.
guitard0g added a comment.
Nit: deleted empty line.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113455/new/
https://reviews.llvm.org/D113455
Files:
clang/lib/CodeGen/CGObjCMac.cpp
clang/test/CodeGenObjC/category-class-empty.m
clang/test/CodeGenObjC/non-lazy-classes.m
Index: clang/test/CodeGenObjC/non-lazy-classes.m
===
--- clang/test/CodeGenObjC/non-lazy-classes.m
+++ clang/test/CodeGenObjC/non-lazy-classes.m
@@ -42,4 +42,7 @@
@implementation E @end
__attribute__((objc_nonlazy_class))
-@implementation E (MyCat) @end
+@implementation E (MyCat)
+-(void) load {
+}
+@end
Index: clang/test/CodeGenObjC/category-class-empty.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/category-class-empty.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O0 -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// PR7431
+
+// CHECK-NOT: @"OBJC_LABEL_CATEGORY_$" = private global [1 x i8*] [i8* bitcast (%struct._category_t* @"_OBJC_$_CATEGORY_A_$_foo"
+
+@interface A
+@end
+__attribute__((objc_direct_members))
+@interface A(foo)
+- (void)foo_myStuff;
+@end
+@implementation A(foo)
+- (void)foo_myStuff {
+}
+@end
+
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -6672,33 +6672,53 @@
}
}
- values.add(emitMethodList(listName, MethodListType::CategoryInstanceMethods,
-instanceMethods));
- values.add(emitMethodList(listName, MethodListType::CategoryClassMethods,
-classMethods));
+ auto instanceMethodList = emitMethodList(
+ listName, MethodListType::CategoryInstanceMethods, instanceMethods);
+ auto classMethodList = emitMethodList(
+ listName, MethodListType::CategoryClassMethods, classMethods);
+ values.add(instanceMethodList);
+ values.add(classMethodList);
+ // Keep track of whether we have actual metadata to emit.
+ bool isEmptyCategory =
+ instanceMethodList->isNullValue() && classMethodList->isNullValue();
const ObjCCategoryDecl *Category =
-Interface->FindCategoryDeclaration(OCD->getIdentifier());
+ Interface->FindCategoryDeclaration(OCD->getIdentifier());
if (Category) {
SmallString<256> ExtName;
-llvm::raw_svector_ostream(ExtName) << Interface->getObjCRuntimeNameAsString() << "_$_"
- << OCD->getName();
-values.add(EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_"
- + Interface->getObjCRuntimeNameAsString() + "_$_"
- + Category->getName(),
-Category->protocol_begin(),
-Category->protocol_end()));
-values.add(EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, false));
-values.add(EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(),
-OCD, Category, ObjCTypes, true));
+llvm::raw_svector_ostream(ExtName)
+<< Interface->getObjCRuntimeNameAsString() << "_$_" << OCD->getName();
+auto protocolList =
+EmitProtocolList("_OBJC_CATEGORY_PROTOCOLS_$_" +
+ Interface->getObjCRuntimeNameAsString() + "_$_" +
+ Category->getName(),
+ Category->protocol_begin(), Category->protocol_end());
+auto propertyList = EmitPropertyList("_OBJC_$_PROP_LIST_" + ExtName.str(),
+ OCD, Category, ObjCTypes, false);
+auto classPropertyList =
+EmitPropertyList("_OBJC_$_CLASS_PROP_LIST_" + ExtName.str(), OCD,
+ Category, ObjCTypes, true);
+values.add(protocolList);
+values.add(propertyList);
+values.add(classPropertyList);
+isEmptyCategory &= protocolList->isNullValue() &&
+ propertyList->isNullValue() &&
+ classPropertyList->isNullValue();
} else {
values.addNullPointer(ObjCTypes.ProtocolListnfABIPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
values.addNullPointer(ObjCTypes.PropertyListPtrTy);
}
- unsigned Size = CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
+ if (isEmptyCategory) {
+// Empty category, don't emit any metadata.
+values.abandon();
+MethodDefinitions.clear();
+return;
+ }
+
+ unsigned Size =
+ CGM.getDataLayout().getTypeAllocSize(ObjCTypes.CategorynfABITy);
values.addInt(ObjCTypes.IntTy, Size);
llvm::GlobalVariable *GCATV =
___
cfe-commits mailing list
cfe-commits
[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
guitard0g added a comment. @ahatanak I don't have commit access, any chance you could commit this for me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113455/new/ https://reviews.llvm.org/D113455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
