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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/Attr.td:290
 
-class LangOpt<string name, bit negated = 0> {
+class LangOpt<string name, bit negated = 0, string customCode = ""> {
   string Name = name;
----------------
I think there's a grand total of one use of `negated`, so you might as well rewrite it to use `customCode`; see below.


================
Comment at: include/clang/Basic/Attr.td:306
+def ObjCNonFragileRuntime : LangOpt<"ObjCNonFragileRuntime", 0,
+                                    "LangOpts.ObjCRuntime.allowsClassStubs()">;
 
----------------
This is not an appropriate name for this `LangOpt`.  How about `ObjCAllowsClassStubs`?


================
Comment at: include/clang/Basic/AttrDocs.td:1100
+implementations defined for them. This attribute is intended for use in
+Swift generated headers for classes defined in Swift.
+    }];
----------------
We should add something here to make it clear what the evolution story with this attribute is.  Presumably you can't add it to an existing class without breaking ABI.  Are there circumstances under which we're willing to guarantee that it can be removed (under a sufficiently-recent deployment target)?


================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3442
+    if (!Code.empty()) {
+      Test += "S.";
+      Test += Code;
----------------
I think the contract with `CustomCode` ought to be that `LangOpts` will be bound as an unqualified name, so that the custom code can be an arbitrary expression in terms of that.  That will allow e.g. compound and negated `LangOpt`s to be defined, like `LangOpts.OpenCL && LangOpts.CPlusPlus`.  So you need to generate `auto &LangOpts = S.LangOpts;` at the top of the function, at least when custom code is present; and you should probably add parens around the code just to be safe.

Alternatively, you could make the contract that `CustomCode` should contain `%0` or some other string that'll be expanded to an expression for the language options, but that sounds pretty complex compared to just binding `LangOpts` in the context.


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

https://reviews.llvm.org/D59628