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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
