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: > > 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. I renamed the functions to names that start with isNonTrivialToPrimitive and removed the enumerator for "Array" from the enum too. https://reviews.llvm.org/D41228 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
