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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
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>;
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6086-6087
+  // This attribute is a no-op without -fobjc-arc.
+  if (!S.getLangOpts().ObjCAutoRefCount)
+    return;
+
----------------
aaron.ballman wrote:
> 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];`
Sure, good point.


================
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());
----------------
aaron.ballman wrote:
> 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.
An important part of this feature is that it could applied to parameters with `#pragma clang attribute` over code that has been audited to be safe. If we require adding `const` to every parameter, then that falls apart :/. 

For better or for worse, this is also consistent with other cases of pseudo-strong in the language, i.e.:
```
void f(NSArray *A) {
  for (id Elem in A)
    Elem = nil; // fine in -fno-objc-arc, error in -fobjc-arc because Elem is implicitly const.
}
```


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

https://reviews.llvm.org/D55865