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: > > > > 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? I think the right name would be isNonTrivialToPrimitiveDefaultInitialize(), and like isDestructedType() it would return an enum that happens to have a zero value (and is thus false) for the trivial case. I think we are likely to add more places that use these methods and enums. For example, if we want to add a diagnostic that warns about memcpy'ing non-trivial struct types, that diagnostic should probably include a note about which field is non-trivial, and that will still basically want to do this same investigation. https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
