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