[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316381: [Sema] Add support for flexible array members in 
Obj-C. (authored by vsapsai).

Changed prior to commit:
  https://reviews.llvm.org/D38773?vs=118990=119943#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38773

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclObjC.cpp
  cfe/trunk/lib/Sema/SemaObjCProperty.cpp
  cfe/trunk/test/Sema/transparent-union.c
  cfe/trunk/test/SemaCXX/flexible-array-test.cpp
  cfe/trunk/test/SemaObjC/flexible-array-arc.m
  cfe/trunk/test/SemaObjC/flexible-array.m
  cfe/trunk/test/SemaObjC/ivar-sem-check-1.m
  cfe/trunk/test/SemaObjCXX/flexible-array.mm

Index: cfe/trunk/lib/Sema/SemaObjCProperty.cpp
===
--- cfe/trunk/lib/Sema/SemaObjCProperty.cpp
+++ cfe/trunk/lib/Sema/SemaObjCProperty.cpp
@@ -1290,6 +1290,14 @@
 // An abstract type is as bad as an incomplete type.
 CompleteTypeErr = true;
   }
+  if (!CompleteTypeErr) {
+const RecordType *RecordTy = PropertyIvarType->getAs();
+if (RecordTy && RecordTy->getDecl()->hasFlexibleArrayMember()) {
+  Diag(PropertyIvarLoc, diag::err_synthesize_variable_sized_ivar)
+<< PropertyIvarType;
+  CompleteTypeErr = true; // suppress later diagnostics about the ivar
+}
+  }
   if (CompleteTypeErr)
 Ivar->setInvalidDecl();
   ClassImpDecl->addDecl(Ivar);
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -14997,67 +14997,78 @@
 //   possibly recursively, a member that is such a structure)
 //   shall not be a member of a structure or an element of an
 //   array.
+bool IsLastField = (i + 1 == Fields.end());
 if (FDTy->isFunctionType()) {
   // Field declared as a function.
   Diag(FD->getLocation(), diag::err_field_declared_as_function)
 << FD->getDeclName();
   FD->setInvalidDecl();
   EnclosingDecl->setInvalidDecl();
   continue;
-} else if (FDTy->isIncompleteArrayType() && Record &&
-   ((i + 1 == Fields.end() && !Record->isUnion()) ||
-((getLangOpts().MicrosoftExt ||
-  getLangOpts().CPlusPlus) &&
- (i + 1 == Fields.end() || Record->isUnion() {
-  // Flexible array member.
-  // Microsoft and g++ is more permissive regarding flexible array.
-  // It will accept flexible array in union and also
-  // as the sole element of a struct/class.
-  unsigned DiagID = 0;
-  if (Record->isUnion())
-DiagID = getLangOpts().MicrosoftExt
- ? diag::ext_flexible_array_union_ms
- : getLangOpts().CPlusPlus
-   ? diag::ext_flexible_array_union_gnu
-   : diag::err_flexible_array_union;
-  else if (NumNamedMembers < 1)
-DiagID = getLangOpts().MicrosoftExt
- ? diag::ext_flexible_array_empty_aggregate_ms
- : getLangOpts().CPlusPlus
-   ? diag::ext_flexible_array_empty_aggregate_gnu
-   : diag::err_flexible_array_empty_aggregate;
-
-  if (DiagID)
-Diag(FD->getLocation(), DiagID) << FD->getDeclName()
-<< Record->getTagKind();
-  // While the layout of types that contain virtual bases is not specified
-  // by the C++ standard, both the Itanium and Microsoft C++ ABIs place
-  // virtual bases after the derived members.  This would make a flexible
-  // array member declared at the end of an object not adjacent to the end
-  // of the type.
-  if (CXXRecordDecl *RD = dyn_cast(Record))
-if (RD->getNumVBases() != 0)
-  Diag(FD->getLocation(), diag::err_flexible_array_virtual_base)
+} else if (FDTy->isIncompleteArrayType() &&
+   (Record || isa(EnclosingDecl))) {
+  if (Record) {
+// Flexible array member.
+// Microsoft and g++ is more permissive regarding flexible array.
+// It will accept flexible array in union and also
+// as the sole element of a struct/class.
+unsigned DiagID = 0;
+if (!Record->isUnion() && !IsLastField) {
+  Diag(FD->getLocation(), diag::err_flexible_array_not_at_end)
+<< FD->getDeclName() << FD->getType() << Record->getTagKind();
+  Diag((*(i + 1))->getLocation(), diag::note_next_field_declaration);
+  FD->setInvalidDecl();
+  EnclosingDecl->setInvalidDecl();
+  continue;
+} else if (Record->isUnion())
+  DiagID = getLangOpts().MicrosoftExt
+   ? 

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 118990.
vsapsai added a comment.

- Address rjmccall review comment, move warn_variable_sized_ivar_visibility to 
DiagnoseVariableSizedIvars.


https://reviews.llvm.org/D38773

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/test/Sema/transparent-union.c
  clang/test/SemaCXX/flexible-array-test.cpp
  clang/test/SemaObjC/flexible-array-arc.m
  clang/test/SemaObjC/flexible-array.m
  clang/test/SemaObjC/ivar-sem-check-1.m
  clang/test/SemaObjCXX/flexible-array.mm

Index: clang/test/SemaObjCXX/flexible-array.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/flexible-array.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Test only flexible array member functionality specific to C++.
+
+union VariableSizeUnion {
+  int s;
+  char c[];
+};
+
+@interface LastUnionIvar {
+  VariableSizeUnion flexible;
+}
+@end
+
+@interface NotLastUnionIvar {
+  VariableSizeUnion flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeUnion' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+
+class VariableSizeClass {
+public:
+  int s;
+  char c[];
+};
+
+@interface LastClassIvar {
+  VariableSizeClass flexible;
+}
+@end
+
+@interface NotLastClassIvar {
+  VariableSizeClass flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeClass' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
Index: clang/test/SemaObjC/ivar-sem-check-1.m
===
--- clang/test/SemaObjC/ivar-sem-check-1.m
+++ clang/test/SemaObjC/ivar-sem-check-1.m
@@ -6,14 +6,15 @@
 @interface INTF
 {
 	struct F {} JJ;
-	int arr[];  // expected-error {{field has incomplete type}}
+	int arr[];  // expected-error {{flexible array member 'arr' with type 'int []' is not at the end of class}}
 	struct S IC;  // expected-error {{field has incomplete type}}
+	  // expected-note@-1 {{next instance variable declaration is here}}
 	struct T { // expected-note {{previous definition is here}}
 	  struct T {} X;  // expected-error {{nested redefinition of 'T'}}
 	}YYY; 
 	FOOBADFUNC;  // expected-error {{field 'BADFUNC' declared as a function}}
 	int kaka;	// expected-note {{previous declaration is here}}
 	int kaka;	// expected-error {{duplicate member 'kaka'}}
-	char ch[];	// expected-error {{field has incomplete type}}
+	char ch[];
 }
 @end
Index: clang/test/SemaObjC/flexible-array.m
===
--- /dev/null
+++ clang/test/SemaObjC/flexible-array.m
@@ -0,0 +1,288 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// # Flexible array member.
+// ## Instance variables only in interface.
+@interface LastIvar {
+  char flexible[];
+}
+@end
+
+@interface NotLastIvar {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Instance variables in implementation.
+@interface LastIvarInImpl
+@end
+@implementation LastIvarInImpl {
+  char flexible[]; // expected-warning {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+}
+@end
+
+@interface NotLastIvarInImpl
+@end
+@implementation NotLastIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@implementation NotLastIvarInImplWithoutInterface { // expected-warning {{cannot find interface declaration for 'NotLastIvarInImplWithoutInterface'}}
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@interface LastIvarInClass_OtherIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+}
+@end
+@implementation LastIvarInClass_OtherIvarInImpl {
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Non-instance variables 

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

rjmccall wrote:
> vsapsai wrote:
> > rjmccall wrote:
> > > "Whether" rather than "If", please.  You should also leave a comment 
> > > about *why* we can't check this here — I assume because you also want to 
> > > complain about the last explicit ivar if there are synthesized ivars?  I 
> > > think we could at least still check this for `@interface` ivars.
> > Will change s/If/Whether/
> > 
> > Main reason for checking elsewhere is to check after ivars are synthesized, 
> > you are right. At some point I had this check done here but for detecting 
> > ivar-after-flexible-array on interface/extension, interface/implementation 
> > border I am relying on chained ObjCIvarDecl. But here I have `ArrayRef > *> Fields` so implementation will be different. I decided that it would be 
> > cleaner to perform the check only in DiagnoseVariableSizedIvars.
> Is there a reason to do any of the checking here, then?
No objective reason. I updated isIncompleteArrayType branch to avoid flexible 
array members rejected at line 15023

```lang=c++
} else if (!FDTy->isDependentType() &&
   RequireCompleteType(FD->getLocation(), FD->getType(),
   diag::err_field_incomplete)) {
```

and here I've added it for consistency. Will move to DiagnoseVariableSizedIvars 
and see if it works fine, don't expect any problems.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

vsapsai wrote:
> rjmccall wrote:
> > "Whether" rather than "If", please.  You should also leave a comment about 
> > *why* we can't check this here — I assume because you also want to complain 
> > about the last explicit ivar if there are synthesized ivars?  I think we 
> > could at least still check this for `@interface` ivars.
> Will change s/If/Whether/
> 
> Main reason for checking elsewhere is to check after ivars are synthesized, 
> you are right. At some point I had this check done here but for detecting 
> ivar-after-flexible-array on interface/extension, interface/implementation 
> border I am relying on chained ObjCIvarDecl. But here I have `ArrayRef *> Fields` so implementation will be different. I decided that it would be 
> cleaner to perform the check only in DiagnoseVariableSizedIvars.
Is there a reason to do any of the checking here, then?


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5226
+def err_objc_variable_sized_type_not_at_end : Error<
+  "field %0 with variable sized type %1 is not at the end of class">;
+def note_next_field_declaration : Note<

rjmccall wrote:
> "Variable sized type" is a bit too close to the C99 variably-sized array type 
> extension.  Maybe "unbounded array type" if you're trying to cover both "int 
> x[];" and "int x[0];"?
> 
> Well, I guess there's some precedent for using this terminology, but ugh.
I took "variable sized type" entirely from

```
def ext_variable_sized_type_in_struct : ExtWarn<
  "field %0 with variable sized type %1 not at the end of a struct or class is"
  " a GNU extension">, InGroup;
```

I'm not covering `int x[0];`. All the changes are for `int x[];` and `struct { 
int x[]; }`



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

rjmccall wrote:
> "Whether" rather than "If", please.  You should also leave a comment about 
> *why* we can't check this here — I assume because you also want to complain 
> about the last explicit ivar if there are synthesized ivars?  I think we 
> could at least still check this for `@interface` ivars.
Will change s/If/Whether/

Main reason for checking elsewhere is to check after ivars are synthesized, you 
are right. At some point I had this check done here but for detecting 
ivar-after-flexible-array on interface/extension, interface/implementation 
border I am relying on chained ObjCIvarDecl. But here I have `ArrayRef 
Fields` so implementation will be different. I decided that it would be cleaner 
to perform the check only in DiagnoseVariableSizedIvars.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5226
+def err_objc_variable_sized_type_not_at_end : Error<
+  "field %0 with variable sized type %1 is not at the end of class">;
+def note_next_field_declaration : Note<

"Variable sized type" is a bit too close to the C99 variably-sized array type 
extension.  Maybe "unbounded array type" if you're trying to cover both "int 
x[];" and "int x[0];"?

Well, I guess there's some precedent for using this terminology, but ugh.



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

"Whether" rather than "If", please.  You should also leave a comment about 
*why* we can't check this here — I assume because you also want to complain 
about the last explicit ivar if there are synthesized ivars?  I think we could 
at least still check this for `@interface` ivars.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Previous discussion on the mailing list can be found at 
http://lists.llvm.org/pipermail/cfe-dev/2017-September/055548.html The 
implementation is not exactly like it was discussed, it has some changes.

There is another case when extra ivar is added at the end, it is for bitfields. 
But bitfields aren't compatible with flexible array members so I decided not to 
include them in tests.

I separated CodeGen change in a separate patch that can be found here 
.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.

Allow Obj-C ivars with incomplete array type but only as the last ivar.
Also add a requirement for ivars that contain a flexible array member to
be at the end of class too. It is possible to add in a subclass another
ivar at the end but we'll emit a warning in this case. Also we'll emit a
warning if a variable sized ivar is declared in class extension or in
implementation because subclasses won't know they should avoid adding
new ivars.

In ARC incomplete array objects are treated as __unsafe_unretained so
require them to be marked as such.

Prohibit synthesizing ivars with flexible array members because order of
synthesized ivars is not obvious and tricky to control. Spelling out
ivar explicitly gives control to developers and helps to avoid surprises
with unexpected ivar ordering.

For C and C++ changed diagnostic to tell explicitly a field is not the
last one and point to the next field. It is not as useful as in Obj-C
but it is an improvement and it is consistent with Obj-C. For C for
unions emit more specific err_flexible_array_union instead of generic
err_field_incomplete.

rdar://problem/21054495


https://reviews.llvm.org/D38773

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/test/Sema/transparent-union.c
  clang/test/SemaCXX/flexible-array-test.cpp
  clang/test/SemaObjC/flexible-array-arc.m
  clang/test/SemaObjC/flexible-array.m
  clang/test/SemaObjC/ivar-sem-check-1.m
  clang/test/SemaObjCXX/flexible-array.mm

Index: clang/test/SemaObjCXX/flexible-array.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/flexible-array.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Test only flexible array member functionality specific to C++.
+
+union VariableSizeUnion {
+  int s;
+  char c[];
+};
+
+@interface LastUnionIvar {
+  VariableSizeUnion flexible;
+}
+@end
+
+@interface NotLastUnionIvar {
+  VariableSizeUnion flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeUnion' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+
+class VariableSizeClass {
+public:
+  int s;
+  char c[];
+};
+
+@interface LastClassIvar {
+  VariableSizeClass flexible;
+}
+@end
+
+@interface NotLastClassIvar {
+  VariableSizeClass flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeClass' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
Index: clang/test/SemaObjC/ivar-sem-check-1.m
===
--- clang/test/SemaObjC/ivar-sem-check-1.m
+++ clang/test/SemaObjC/ivar-sem-check-1.m
@@ -6,14 +6,15 @@
 @interface INTF
 {
 	struct F {} JJ;
-	int arr[];  // expected-error {{field has incomplete type}}
+	int arr[];  // expected-error {{flexible array member 'arr' with type 'int []' is not at the end of class}}
 	struct S IC;  // expected-error {{field has incomplete type}}
+	  // expected-note@-1 {{next instance variable declaration is here}}
 	struct T { // expected-note {{previous definition is here}}
 	  struct T {} X;  // expected-error {{nested redefinition of 'T'}}
 	}YYY; 
 	FOOBADFUNC;  // expected-error {{field 'BADFUNC' declared as a function}}
 	int kaka;	// expected-note {{previous declaration is here}}
 	int kaka;	// expected-error {{duplicate member 'kaka'}}
-	char ch[];	// expected-error {{field has incomplete type}}
+	char ch[];
 }
 @end
Index: clang/test/SemaObjC/flexible-array.m
===
--- /dev/null
+++ clang/test/SemaObjC/flexible-array.m
@@ -0,0 +1,288 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// # Flexible array member.
+// ## Instance variables only in interface.
+@interface LastIvar {
+  char flexible[];
+}
+@end
+
+@interface NotLastIvar {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Instance variables in implementation.
+@interface LastIvarInImpl
+@end
+@implementation LastIvarInImpl {
+  char flexible[]; // expected-warning {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+}
+@end
+
+@interface NotLastIvarInImpl
+@end
+@implementation NotLastIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their