[PATCH] D45310: Warn about memcpy'ing non-trivial C structs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/NonTrivialCStructTypeVisitor.h:30
+    if (asDerived().getContext().getAsArrayType(FT))
+      return asDerived().visitArray(DK, FT, std::forward<Ts>(Args)...);
+
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Should you have this pass the array type down?  And is it really important to do this in the generic visitor?  It seems like something you could do in an IRGen subclass.
> > The subclasses in CGNonTrivialStruct.cpp need the size and the element type of the array to be passed to visitArray, so I think we have to pass the array type to visitArray. I guess it's possible to move this to the subclasses, but then the visit methods in the subclasses have to check whether the type is an array or not. I think we had a discussion on how arrays should be handled in this review: https://reviews.llvm.org/D41228.
> > 
> > But perhaps you have a better idea in mind?
> Well, you could "override" the visit method in the subclass, e.g.:
> 
>   template <class Derived, class RetTy = void>
>   class CGDestructedTypeVisitor : public DestructedTypeVisitor<Derived, RetTy> {
>     using super = DestructedTypeVisitor<Derived, RetTy>;
> 
>   public:
>     using super::asDerived;
>     using super::visit;
> 
>     template <class... Ts>
>     RetTy visit(QualType::DestructionKind DK, QualType FT, Ts &&... Args) {
>       if (asDerived().getContext().getAsArrayType(FT))
>         return asDerived().visitArray(DK, FT, std::forward<Ts>(Args)...);
> 
>       return super::visit(DK, FT, std::forward<Ts>(Args)...);
>     }
>   };
> 
> It's a bit more boilerplate, but I really feel like the array logic doesn't belong in the generic visitor.
> 
> About the array type: I wasn't trying to suggest that you should pass the element type to visitArray, I was suggesting you could just pass the array type as an `ArrayType*`, since that's what `visitArray` actually wants.
I incorporated both of your suggestions. I renamed the overloaded method 'visit' to 'visitWithKind' so that the 'visit' method that doesn't take the DestructionKind doesn't get hidden by the one that takes DestructionKind and is defined in the derived classes. There are a couple of places in CGNonTrivialStruct.cpp that call the former method.


Repository:
  rC Clang

https://reviews.llvm.org/D45310