[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, the tests look great.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3919
+in particular, this means that it cannot override a superclass method or satisfy
+a protocol requirement.
+
----------------
Please add a new paragraph here:

  Because a direct method cannot be overridden, it is an error to perform
  a ``super`` message send of one.

And you should test that.  (I noticed this because you had an `assert(!IsSuper);` in IRGen, which was both a good idea and also begging for a justification. :))


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4081
+    //
+    // TODO: make sure that when the inliner kicks in we do this once only
+    result = GeneratePossiblySpecializedMessageSend(
----------------
I don't think the first half of this comment is necessary anymore.

The second half should be clearer; I'd suggest something like:

  TODO: If this method is inlined, the caller might know that `self` is already initialized;
  for example, it might be an ordinary Objective-C method which always receives an
  initialized `self`, or it might have just forced initialization on its own.  We should
  find a way to eliminate this unnecessary initialization in such cases in LLVM.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4087
+
+    ReceiverCanBeNull = isWeakLinkedClass(OID);
+  }
----------------
The assumption here is that a direct class method can never be invoked on a nullable value, like a `Class`.  I think that's true, but it's worth making that explicit in a comment.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4114
+      CGF.EmitBranchThroughCleanup(CGF.ReturnBlock);
+    }
+    // }
----------------
Please branch to the return block in both cases.


================
Comment at: clang/test/CodeGenObjC/direct-method.m:25
+  // CHECK-NEXT: store %0* %self, %0** %self.addr,
+  // CHECK-NEXT: store i8* %_cmd, i8** %_cmd.addr,
+
----------------
The IR names of local values aren't emitted in release builds of the compiler, so you need to use FileCheck variables for these allocas the same way that you do for RETVAL and SELF.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69991/new/

https://reviews.llvm.org/D69991