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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 21 05:54:03 PDT 2019


aaron.ballman added a comment.

You should also add Sema tests that ensure the attribute applies to the expected AST nodes, is diagnosed appropriately when applied to something it shouldn't be applied to, accepts no args, etc. Basically, all of the semantic places we could warn on should have test coverage, as well as demonstrations of correct usage.



================
Comment at: include/clang/Basic/Attr.td:1798
 
+def ObjCClassStub: Attr {
+  let Spellings = [Clang<"objc_class_stub">];
----------------
Formatting is a smidge off here -- should be `ObjCClassStub : Attr` with the extra whitespace.


================
Comment at: include/clang/Basic/Attr.td:1802
+  let Documentation = [ObjCClassStubDocs];
+}
+
----------------
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.


================
Comment at: include/clang/Basic/AttrDocs.td:1116
+def ObjCClassStubDocs : Documentation {
+    let Category = DocCatFunction;
+    let Content = [{
----------------
This seems like the wrong category -- the attribute doesn't apply to functions.


================
Comment at: include/clang/Basic/AttrDocs.td:1118
+    let Content = [{
+This attribute specifies that the Objective-C class to which it applies has dynamically-allocated metadata. Classes annotated with this attribute cannot be subclassed.
+    }];
----------------
rjmccall wrote:
> You should probably check that the user doesn't try to subclass classes annotated with this attribute, then. :)
Try to keep the docs wrapped to the usual 80-col limit.

I think this could use a bit more exposition, or links to explain stuff. Based on the small amount here, I still have no idea why I would use this attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59628





More information about the cfe-commits mailing list