rjmccall added inline comments.
================
Comment at: include/clang/AST/Type.h:1102
+ PDK_Struct // non-trivial C struct.
+ };
+
----------------
This is unused.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1565
// For all other types, the memcpy is fine.
return std::make_pair(BlockCaptureEntityKind::None, Flags);
----------------
Please restructure this code to use the result of
isNonTrivialToPrimitiveCopy(), preferably in a way that involves an exhaustive
switch. That should be as easy as breaking out in the PCK_Strong case and in
the PCK_None case when !isObjCRetainableType().
================
Comment at: lib/CodeGen/CGBlocks.cpp:1773
+ return std::make_pair(BlockCaptureEntityKind::NonTrivialCStruct,
+ BlockFieldFlags());
+
----------------
Same as above with using the result of isDestructedType().
================
Comment at: lib/CodeGen/CGBlocks.cpp:2070
+ bool needsDispose() const override {
+ return VarType.isDestructedType() == QualType::DK_nontrivial_c_struct;
+ }
----------------
Just isDestructedType() should be fine.
================
Comment at: lib/CodeGen/CGDecl.cpp:1299
+ return;
+ }
+
----------------
Again, it would be nice to restructure this code to use the result of
isNonTrivialToPrimitiveDefaultInitialize() exhaustively.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:38
+ template <class Visitor, class... Ts>
+ static void visit(Visitor &V, QualType FT, bool IsVolatile, unsigned Offset,
+ Ts... Args) {
----------------
These visitors would be re-usable components outside of IRGen if they just took
a QualType and then forwarded an arbitrary set of other arguments. I would
suggest structuring them like this:
template <class Derived, class RetTy> struct DestructedTypeVisitor {
Derived &asDerived() { return static_cast<Derived&>(*this); }
template <class... Ts>
RetTy visit(QualType Ty, Ts &&...Args) {
return asDerived().visit(Ty.isDestructedType(), Ty,
std::forward<Ts>(Args)...);
}
template <class... Ts>
RetTy visit(QualType::DestructionKind DK, QualType Ty, Ts &&...Args) {
switch (DK) {
case QualType::DK_none: return asDerived().visitTrivial(Ty,
std::forward<Ts>(Args)...);
case QualType::DK_cxx_destructor: return
asDerived().visitCXXDestructor(Ty, std::forward<Ts>(Args)...);
...
}
}
};
You can then pretty easily add the special behavior for IsVolatile and arrays
(when DK is not DK_none) in an IRGen-specific subclass. visitArray would take
a DK and then recurse by calling back into visit with that same DK.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:55
+ break;
+ default:
+ break;
----------------
Better not to have a default case here. You can add an assert for the C++ case
that it should never get here.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:92
+ }
+};
+
----------------
This entire template is dead.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:110
+ unsigned FOffset =
+ Offset + RL.getFieldOffset(FieldNo++) / Ctx.getCharWidth();
+ FieldVisitor::visit(getDerived(), FT,
----------------
Please pass around offsets as CharUnits until you actually need to interact
with LLVM.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:131
+
+template <class Derived, bool IsMove> struct BinaryFuncStructVisitor {
+ typedef BinaryFieldVisitor FieldVisitorTy;
----------------
I think you should just call this CopyStructVisitor.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:145
+ if (PCK)
+ getDerived().flushTrivialFields(Args...);
+
----------------
This would also be a good place to check for arrays so you don't have to do it
in every non-trivial case below.
There's a couple other places in this file where I would suggest the same thing.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:289
+/// Return an address with the specified offset from the passed address.
+static Address getAddrWithOffset(Address Addr, unsigned Offset,
+ CodeGenFunction &CGF) {
----------------
Offset should definitely be a CharUnits here.
================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:432
+ // If the special function already exists in the module, return it.
+ if (llvm::Function *F = CGM.getModule().getFunction(FuncName))
+ return F;
----------------
You should at least make sure this has the right function type; it's possible,
even if not really allowed, to declare a C function that collides with one of
these names.
================
Comment at: lib/CodeGen/CodeGenFunction.h:3407
+ QualType QT, bool IsVolatile);
+ void callCStructDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+
----------------
I think we're moving towards generally taking LValues instead of Addresses
unless you're talking about a very low-level routine.
https://reviews.llvm.org/D41228
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
