aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Decl.h:1357
+ bool isARCPseudoStrong() const { return VarDeclBits.ARCPseudoStrong; }
+ void setARCPseudoStrong(bool ps) { VarDeclBits.ARCPseudoStrong = ps; }
----------------
Can you update `ps` to be `PS` for naming conventions?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3492
+def warn_ignored_objc_externally_retained : Warning<
+ "'objc_externally_retained' can only be applied to strong retainable "
+ "object pointer types with automatic storage">, InGroup<IgnoredAttributes>;
----------------
This wording isn't quite right -- it doesn't apply to types, it applies to
variables of those types. How about: `... to local variables or parameters of
strong, retainable object pointer type`? or something along those lines?
================
Comment at: clang/lib/CodeGen/CGDecl.cpp:2102
// init method then we don't want to retain it.
- if (D.isARCPseudoStrong()) {
- const ObjCMethodDecl *method = cast<ObjCMethodDecl>(CurCodeDecl);
- assert(&D == method->getSelfDecl());
- assert(lt == Qualifiers::OCL_Strong);
- assert(qs.hasConst());
- assert(method->getMethodFamily() != OMF_init);
- (void) method;
- lt = Qualifiers::OCL_ExplicitNone;
+ if (auto *method = dyn_cast<ObjCMethodDecl>(CurCodeDecl)) {
+ if (D.isARCPseudoStrong() && method->getSelfDecl() == &D) {
----------------
method -> Method per naming conventions.
================
Comment at: clang/lib/CodeGen/CGObjC.cpp:158
- // Perform the actual initialialization of the array(s).
+ // Perform the actual initialization of the array(s).
for (uint64_t i = 0; i < NumElements; i++) {
----------------
Good catch, but not really part of this patch -- feel free to commit separately.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6086-6087
+ // This attribute is a no-op without -fobjc-arc.
+ if (!S.getLangOpts().ObjCAutoRefCount)
+ return;
+
----------------
I think we should warn that the attribute is ignored in this case (because
we're dropping the attribute from things like pretty printing, ast dumps, etc).
Instead of handling this semantically, you can do it declaratively by using the
`LangOpts` field in Attr.td. Something like `let LangOpts = [ObjCAutoRefCount];`
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6117
+ // to ensure that the variable is 'const' so that we can error on
+ // modification, which can otherwise overrelease.
+ VD->setType(Ty.withConst());
----------------
overrelease -> over-release
Btw, this isn't a bit of a hack, it's a huge hack, IMO. Instead, can we simply
err if the user hasn't specified `const` explicitly? I'm not a fan of "we're
hiding this rather important language detail behind an attribute that can be
ignored". This is especially worrisome because it means the variable is const
only when ARC is enabled, which seems like surprising behavioral differences
for that compiler flag.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:11227
+ // - Objective-C externally_retained attribute.
+ } else if (declRef->getDecl()
+ ->hasAttr<ObjCExternallyRetainedAttr>()) {
----------------
`var->hasAttr<>` instead?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55865/new/
https://reviews.llvm.org/D55865
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
