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 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. So QualType::hasNonTrivialToDefaultInitializeStruct should be renamed to QualType::hasPrimitiveDefaultInitializeStruct and RecordDecl::isNonTrivialToPrimitiveDefaultInitialize to RecordDecl::isPrimitiveDefaultInitialize, for example? Also, I realized that, after I made the changes that removed the NonTrivial flags from RecordDecl, CGNonTrivialStruct.cpp is the only user of enum NonTrivialCopyKind and the following methods of QualType (Sema::isValidVarArgType calls nonTrivialToDestroy, but it should call methods like hasNonTrivialToDestroyStruct that detect non-trivial C structs instead): nonTrivialToDefaultInitialize nonTrivialToCopy nonTrivialToDestructiveMove nonTrivialToDestroy Maybe we should make these enums and functions local to CGNonTrivialStruct.cpp to avoid adding something that is not used outside CGNonTrivialStruct.cpp? https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
