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 DestructionKind. Is it important > > here? You can't actually do anything with that information. > > > > Regarding your naming question, I wonder if it would be useful to just > > invent a new term here, something meaning "non-trivial" but without the C++ > > baggage. Maybe just "PrimitiveCopyKind", with the understanding that C++ > > can never do a "primitive" copy? And for consistency with DestructionKind, > > maybe you should lowercase the second words. > > > > I'm not sure how isNonTrivialToCopy is supposed to be used. Where does the > > function come from? And why is a context required when it isn't required > > for isDestructedType? > Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to distinguish > the different types of fields in a C struct. Currently, there are four types > that are handled by visitField functions in CGNonTrivialStruct.cpp: > > - struct field > - array field > - __strong field > - trivial field that can be copied using memcpy > > I thought using enums would make the code easier to read, but of course it's > possible to remove them and instead just check the field types calling > functions like getAsArrayType or getAs<RecordType>. ASTContext is passed to > isNonTrivialToCopy so that it can call ASTContext::getAsArrayType to > determine whether the type is an array. Maybe there is another way to detect > an array that doesn't require ASTContext? > > As for the naming, PrimitiveCopyKind seems find if we want to distinguish it > from C++ non-trivial classes (I don't have a better name). If we are going to > call non-trivial C structs primitiveCopyKind, what should we call the C > structs that are trivial (those that can be copied using memcpy)? I think "trivial" is a reasonable kind of primitive-copy semantics. Basically, we're saying that all complete object types other than non-trivial C++ types have some sort of primitive-copy semantics, and this enum tells us what those semantics are. DestructionKind has to deal with arrays as well, but it does so by just reporting the appropriate value for the underlying element type without mentioning that it's specifically an *array* of the type. I think that's a reasonable design, since most places do not need to distinguish arrays from non-arrays. IRGen does, but only when you're actually finally emitting code; prior to that (when e.g. deciding that a field is non-trivial to copy) you can just use the enum value. You can do without getAsArrayType the same way that isDestructedType() does: the more complex operation is only necessary in order to preserve qualifiers on the element type, but that's unnecessary for this operation because the array type itself is considered to have the same qualifiers as its element type. That is, you can ask the original type whether it's __strong/__weak-qualified without having to specifically handle array types, and once you've done all of those queries, you can use the "unsafe" operations to drill down to the element type because you no longer care about qualification. https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
