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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3783
+parameters or local variables in ARC mode. When applied to parameters, it causes
+clang to omit the implicit calls to objc_retain and objc_release on function
+entry and exit respectivly. When applied to local variables, it causes clang to
----------------
These function names should be `typeset in mono`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3784
+clang to omit the implicit calls to objc_retain and objc_release on function
+entry and exit respectivly. When applied to local variables, it causes clang to
+omit the call to retain on variable initialization, and release when the
----------------
Typo


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3787
+variable goes out of scope. Parameters and variables annotated with this
+attribute are implicitly ``const``.
+
----------------
You should add a paragraph contrasting this with ``__unsafe_unretained``.  For example:

  Unlike an `__unsafe_unretained` variable, an externally-retained variable remains
  semantically `__strong`.  This affects features like block capture which copy the
  current value of the variable into a capture field of the same type: the block's
  capture field will still be `__strong`, and so the value will be retained as part of
  capturing it and released when the block is destroyed.  It also affects C++ features
  such as lambda capture, `decltype`, and template argument deduction.

You should also describe this attribute in AutomaticReferenceCounting.rst.
You can add it in a new `arc.misc.externally_retained` section immediately above
`arc.misc.self`.  You can borrow some of the existing wording from the two
following sections, `arc.misc.self` and `arc.misc.enumeration`, and then make
those sections refer back to the new concept.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3789
+
+This attribute is meant to be used to opt-out of the additional safety that ARC
+provides when the programmer knows that the safety isn't necessary. You can read
----------------
The hyphen is unneeded here.


================
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>;
----------------
erik.pilkington wrote:
> rjmccall wrote:
> > erik.pilkington wrote:
> > > aaron.ballman wrote:
> > > > 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?
> > > Sure, that is a bit more clear.
> > I'd split this into two diagnostics:
> > - "...can only be applied to local variables and parameters of retainable type" (if the type or decl kind is wrong)
> > - "...can only be applied to variables with strong ownership" (if the qualifier is wrong)
> Sure, I guess a lot of information is crammed into this one. I coalesced the two you suggested into one with a %select.
WFM


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:807
+    // that we omit the retain, and causes non-autoreleased return values to be
+    // immediatly released.
+    LLVM_FALLTHROUGH;
----------------
typo


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:2334
+      // If a parameter is pseudo-strong, either because of the attribute
+      // objc_externally_retained or because its 'self' in an non-init method,
+      // then we can omit the implicit retain.
----------------
I wouldn't enumerate the cases in this comment.  You might want to enumerate them in a comment on `isARCPseudoStrong()`, though.


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

https://reviews.llvm.org/D55865