[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

[Aaron Ballman via Phabricator via cfe-commits]

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