[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

sberg added a comment. It looks like this caused a clang-cl regression https://bugs.llvm.org/show_bug.cgi?id=37146 "clang-cl emits special functions for non-trivial C-structs ('__destructor_8') introduced for Objective-C". Repository: rL LLVM https://reviews.llvm.org/D41228 _

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

This revision was automatically updated to reflect the committed changes. Closed by commit rL326307: [ObjC] Allow declaring __strong pointer fields in structs in Objective-C (authored by ahatanak, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! This looks ready; thank you for your patience in working this out. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PC

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PCK_Struct // non-trivial C struct. + }; rjmccall wrote: > These should all be /// documentation comments, and they mostly shouldn't >

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 135703. ahatanak marked 9 inline comments as done. ahatanak added a comment. Address review comments. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.h include/clang/Basic/Diagnosti

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1121 + /// after it is moved, as opposed to a truely destructive move in which the + /// source object is placed in an uninitialized state. + PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1121 + /// after it is moved, as opposed to a truely destructive move in which the + /// source object is placed in an uninitialized state. + PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PCK_Struct // non-trivial C struct. + }; These should all be /// documentation comments, and they mostly shouldn't talk about fields si

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 135309. ahatanak marked 14 inline comments as done. ahatanak added a comment. Address review comments. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.h include/clang/Basic/Diagnost

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1423 + if (capturedByInit) +drillIntoBlockVariable(*this, lvalue, cast(D)); + rjmccall wrote: > The "delay" is that we generally handle `__block` captures within > initializers by de

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3619 +return NonTrivialToPrimitiveDestructiveMove; + } + Please document that this means a C++-style destructive move that leaves the source object in a validly-initialized state. ==

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1137 +return isNonTrivialToPrimitiveDestroy() == PCK_Struct; + } + rjmccall wrote: > Are these four queries really important enough to provide API for them on > QualType? I removed these f

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 134430. ahatanak marked 14 inline comments as done. ahatanak added a comment. Address review comments. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.h include/clang/Basic/Diagnost

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1102 +PDK_Struct // non-trivial C struct. + }; + This is unused. Comment at: lib/CodeGen/CGBlocks.cpp:1565 // For all other types, the memcpy is fine. return

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 133994. ahatanak marked 8 inline comments as done. ahatanak added a comment. Address review comments and feedback I got from John offline. The main changes are in CGNonTrivialStruct.cpp. I cleaned up the class hierarchy and used variadic template functions

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1148 +DK_objc_weak_lifetime, +DK_c_struct_strong_field }; ahatanak wrote: > rjmccall wrote: > > I don't think you want to refer to the fact that the C struct specifically > > contain

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1148 +DK_objc_weak_lifetime, +DK_c_struct_strong_field }; rjmccall wrote: > I don't think you want to refer to the fact that the C struct specifically > contains a __strong field her

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3529 + /// Basic properties of non-trivial C structs. + bool HasStrongObjCPointer : 1; + Is it interesting to track all the individual reasons that a struct is non-trivial at the struct level

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 129359. ahatanak marked an inline comment as done. ahatanak added a comment. In EmitCallArg and EmitParmDecl, use the existing code for Microsoft C++ ABI to determine whether the argument should be destructed in the callee. Also, add a test case that checks

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak marked an inline comment as done. ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; rjmccall wrote: > ahatanak wrote: > >

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 128061. ahatanak added a comment. Rename functions. Check whether a non-trivial type passed to Sema::isValidVarArgType is a struct. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.h

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall wrote: > >

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; rjmccall wrote: > ahatanak wrote: > > rjmccall wrote: > > > We don't actually di

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; ahatanak wrote: > rjmccall wrote: > > We don't actually distinguish arrays in De

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; rjmccall wrote: > We don't actually distinguish arrays in DestructionKind. Is i

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; We don't actually distinguish arrays in DestructionKind. Is it important here?

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 127818. ahatanak added a comment. - Improved IRGen for copying trivial fields in a non-trivial C struct. IRGen in CGNonTrivialStruct.cpp now calls a single memcpy if there are consecutive trivial fields in a struct (it does something similar to what FieldMe

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak marked 2 inline comments as done. ahatanak added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:315 +} + } } ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall wrote: > > > > Do these functions have a memcpy as a precondit

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak marked 2 inline comments as done. ahatanak added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:315 +} + } } rjmccall wrote: > ahatanak wrote: > > rjmccall wrote: > > > Do these functions have a memcpy as a precondition? I would expect the

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 127591. ahatanak added a comment. Address review comments. Changed the mangling of special functions to make it a bit easier to read. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1421 +destroyer = CodeGenFunction::destroyNonTrivialCStruct; +cleanupKind = getARCCleanupKind(); +break; ahatanak wrote: > rjmccall wrote: > > This can only be getARCCleanupKind() if i

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 127456. ahatanak marked 5 inline comments as done. ahatanak added a comment. Update patch. https://reviews.llvm.org/D41228 Files: docs/LanguageExtensions.rst include/clang/AST/Decl.h include/clang/AST/Type.h include/clang/Basic/DiagnosticSemaKinds.

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak marked 9 inline comments as done. ahatanak added a comment. Address review comments. Comment at: lib/CodeGen/CGDecl.cpp:1421 +destroyer = CodeGenFunction::destroyNonTrivialCStruct; +cleanupKind = getARCCleanupKind(); +break; rjmccall wrote:

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added a comment. I accidentally hit 'send' too early on my review, so here's part two. I still haven't fully reviewed the new IRGen file. Comment at: lib/CodeGen/CodeGenCStruct.cpp:27 + FK_Array // array that has non-trivial elements. +}; +} There

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added a comment. You should add a __has_feature check for this (__has_feature(objc_arc_fields)?), as well as documentation. The documentation would go in the ARC spec. Comment at: include/clang/AST/Decl.h:3575 + /// Functions to query basic properties of non-trivia

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 127233. ahatanak added a comment. Insert an underscore before src-alignment in the BNF rule for alignment-info. ::= ["_" ] https://reviews.llvm.org/D41228 Files: include/clang/AST/Decl.h include/clang/AST/Type.h include/clang/Basic/DiagnosticSemaK

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak updated this revision to Diff 127231. ahatanak added a comment. Fixed a bug where the code gets stuck in an infinite loop when a struct with an array field is passed to FieldInfoToString. Encoded the offset of an array in a struct into the function name. Also, added comments that descri

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak added a comment. I just found that the code that creates the mangled name for a special function is not correct. Two structs with different record layouts can end up having functions that have the same name. I'll fix the bug and update the patch today. https://reviews.llvm.org/D41228

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

ahatanak created this revision. ahatanak added reviewers: rjmccall, doug.gregor, rsmith. Herald added a subscriber: mgorny. ObjectiveC ARC in C mode currently disallows having __strong and __weak pointers in structs. This patch takes the first step towards lifting that restriction. In order to p