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

[John McCall via Phabricator via cfe-commits]

rjmccall 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)...);
+
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D45310