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
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:
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.
+
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
>
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
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;
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;
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
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
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
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.
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
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
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
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
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
> >
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
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
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
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:
>
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
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:
> >
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
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
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
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?
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
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
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
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
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
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
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
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.
+};
+}
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
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
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
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.
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
39 matches
Mail list logo
