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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
