[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

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