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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/Type.h:1137
+    return isNonTrivialToPrimitiveDestroy() == PCK_Struct;
+  }
+
----------------
rjmccall wrote:
> Are these four queries really important enough to provide API for them on QualType?
I removed these functions.


================
Comment at: lib/CodeGen/CGDecl.cpp:1299
+    return;
+  }
+
----------------
rjmccall wrote:
> Again, it would be nice to restructure this code to use the result of isNonTrivialToPrimitiveDefaultInitialize() exhaustively.
It looks like this code needs restructuring, but I didn't understand how I can use isNonTrivialToPrimitiveDefaultInitialize() here. The only change I made here is that drillIntoBlockVariable is called if the variable is byref. This fixes a bug where a non-trivial struct variable declared as __block wasn't initialized correctly.

Also, in the process, I found that when a c++ struct declared as a __block is captured by its initializer, an assertion fails (see test case in test/CodeGenObjCXX/arc-blocks.mm). I fixed the bug in CodeGenFunction::EmitExprAsInit, but I'm not sure that's the right way to fix it. I'm not sure what the comment "how can we delay here if D is captured by its initializer" means.


================
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) {
----------------
rjmccall wrote:
> 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.
I created three visitor classes that can be used outside of IRGen,


================
Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:55
+      break;
+    default:
+      break;
----------------
rjmccall wrote:
> Better not to have a default case here.  You can add an assert for the C++ case that it should never get here.
I added llvm_unreachable in StructVisitor's methods visitWeak and visitCXXDestructor.


https://reviews.llvm.org/D41228