[PATCH] D59628: Add support for __attribute__((objc_class_stub))

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59628#1446626 <https://reviews.llvm.org/D59628#1446626>, @slavapestov wrote:

> I don't know what the etiquette is around here about pinging reviewers for a re-review, but this CL is ready for another look. Your feedback is much appreciated!


Thanks for letting us know you think you've addressed all the feedback. We usually recommend pinging once a week if there's no activity on a review thread, but it's no problem to ping sooner when you have more information.



================
Comment at: include/clang/Basic/Attr.td:1802
+  let Documentation = [ObjCClassStubDocs];
+}
+
----------------
slavapestov wrote:
> aaron.ballman wrote:
> > Does this attribute make sense outside of ObjC? e.g., should it require an ObjC compiland? If it should only be used in ObjC, then it should have a `LangOpts` field.
> The other ObjC attributes, such as ObjCSubclassingRestricted, do not have a LangOpts. Do you want me to add a LangOpts field to those too? Or is it unnecessary since they're already restricted to InterfaceDecls?
> The other ObjC attributes, such as ObjCSubclassingRestricted, do not have a LangOpts. Do you want me to add a LangOpts field to those too? Or is it unnecessary since they're already restricted to InterfaceDecls?

Don't feel obligated, but if you wanted to post a follow-up patch that adds this to the other ObjC attribute, I think it would be a good change to make. It's not strictly required, but I think the diagnostics are a bit more clear when we restrict the language options.


================
Comment at: include/clang/Basic/AttrDocs.td:1116
+def ObjCClassStubDocs : Documentation {
+    let Category = DocCatFunction;
+    let Content = [{
----------------
slavapestov wrote:
> aaron.ballman wrote:
> > This seems like the wrong category -- the attribute doesn't apply to functions.
> Would DocCatType make more sense? Would you like me to change ObjCRuntimeVisible and a handful of other miscategorized attributes too?
Oye, it looks like we have a lot of inconsistent categories currently. It's not really a type attribute, it's a declaration attribute, but we don't have such a notion yet. Go ahead and leave this as `DocCatType` for now; I'll see about cleaning this up in a follow-up.


================
Comment at: include/clang/Basic/ObjCRuntime.h:437-443
+    case FragileMacOSX: return false;
+    case GCC: return false;
+    case MacOSX: return true;
+    case GNUstep: return false;
+    case ObjFW: return false;
+    case iOS: return true;
+    case WatchOS: return true;
----------------
I'd prefer if you grouped these cases and did it like:
```
switch (getKind()) {
case FragileMacOSX:
case GCC:
case GNUstep:
case ObjFW:
  return false;
case MacOSX:
case iOS:
case WatchOS:
  return true;
}
```


================
Comment at: lib/CodeGen/CGObjCMac.cpp:7269
+CGObjCNonFragileABIMac::GetClassGlobalForClassRef(const ObjCInterfaceDecl *ID) {
+  auto *ClassGV = GetClassGlobal(ID, /*metaclass*/ false, NotForDefinition);
+
----------------
Please don't use `auto` here, the type is not spelled out in the initialization.


================
Comment at: lib/CodeGen/CGObjCMac.cpp:7347
   if (!Entry) {
-    auto ClassGV = GetClassGlobal(ID, /*metaclass*/ false, NotForDefinition);
-    Entry = new llvm::GlobalVariable(CGM.getModule(), ObjCTypes.ClassnfABIPtrTy,
+    auto ClassGV = GetClassGlobalForClassRef(ID);
+    Entry = new llvm::GlobalVariable(CGM.getModule(), ClassGV->getType(),
----------------
Don't use `auto` here either.


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4097
 
+      if (IDecl->hasAttr<ObjCClassStubAttr>()) {
+        Diag(IC->getLocation(), diag::err_implementation_of_class_stub);
----------------
Elide braces.


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4131-4133
+      if (!getLangOpts().ObjCRuntime.allowsClassStubs()) {
+        Diag(IntfDecl->getLocation(), diag::err_class_stub_not_supported);
+      }
----------------
This should be done in Attr.td. You'll need to add a new LangOpt subclass (around line 298 or so are examples), and then add it to the `LangOpts` array when defining the attribute. Then you can remove this code as well as the new diagnostic.


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4134
+      }
+      if (!IntfDecl->hasAttr<ObjCSubclassingRestrictedAttr>()) {
+        Diag(IntfDecl->getLocation(), diag::err_class_stub_subclassing_mismatch);
----------------
Elide braces.


================
Comment at: test/CodeGenObjC/class-stubs.m:2
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -Wno-objc-root-class -emit-llvm -o - %s | \
+// RUN: FileCheck %s
+
----------------
Hmm, this looks suspicious -- I think it's more clear to just bump this to the previous line rather than wonder if the RUN: command will confuse things.


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

https://reviews.llvm.org/D59628