[PATCH] D65665: Support for attributes on @class declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't think I know enough about ObjC semantics to say much about the language design aspects of this patch.



================
Comment at: clang/include/clang/Parse/Parser.h:1496
+  DeclGroupPtrTy ParseObjCAtClassDeclaration(SourceLocation atLoc,
+                                             ParsedAttributes &Attrs);
   Decl *ParseObjCAtInterfaceDeclaration(SourceLocation AtLoc,
----------------
Slight preference for using `ParsedAttributesWithRange` instead when making new interfaces -- we often need the range when trying to prohibit attributes in places, or for other diagnostic purposes.

However, this may not be feasible depending on where you're getting the attributes from (some parts of the parser are still unfortunately deficient in this regard).


================
Comment at: clang/lib/Parse/ParseObjc.cpp:124
 /// objc-class-declaration:
 ///    '@' 'class' objc-class-forward-decl (',' objc-class-forward-decl)* ';'
 ///
----------------
Comment seems out of date now.


================
Comment at: clang/test/CodeGenObjC/objc-asm-attribute-test.m:78
 // CHECK: [[IVAR:%.*]] = load i64, i64* @"OBJC_IVAR_$_MySecretNamespace.Message.MyIVAR"
+
----------------
Can probably drop this spurious newline.


================
Comment at: clang/test/SemaObjC/attr-forward-class.m:11-12
+
+// FIXME: it'd be nice to point to the declaration with the attribute if its
+// inherited.
+ at interface FwdDefined // expected-note{{introduced in macOS 1000 here}}
----------------
Do you have plans to fix this? If so, I'm fine with a FIXME for now, but I think the note is essentially a bug as it stands.


================
Comment at: clang/test/SemaObjC/attr-forward-class.m:27
+
+// expected-error at +1{{postfix attributes are not allowed on Objective-C directives}}
+ at class __attribute__((availability(macos, introduced=1000))) InTheMiddle;
----------------
What is a "postfix attribute"? I would have assumed this was attempting to add a type attribute rather than a declaration attribute (based on the usual C and C++ semantics), so this diagnostic is a bit unhelpful.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65665