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