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>;
----------------
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.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1946
+ if (isInit && isPseudoStrong)
+ Lifetime = Qualifiers::OCL_ExplicitNone;
+
----------------
rjmccall wrote:
> Where are we allowing assignments into pseudo-strong variables?
We're not, I removed this.
================
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:
> erik.pilkington wrote:
> > 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.
> > }
> > ```
> > 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 :/.
>
> Thanks for letting me know about that use case; it adds some helpful context.
> However, I don't see why this falls apart -- if you're auditing the code, you
> could add the `const` qualifiers at that time, couldn't you?
>
> Alternatively, if we warned instead of erred on the absence of `const`, then
> this could be automated through clang-tidy by checking declarations that are
> marked with the attribute but are not marked `const` and use the fix-it
> machinery to update the code.
>
> > For better or for worse, this is also consistent with other cases of
> > pseudo-strong in the language,
>
> Yes, but it's weird behavior for an attribute. The attribute is applied to
> the *declaration* but then it silently modifies the *type* as well, but only
> for that one declaration (which is at odds with the usual rules for
> double-square bracket attributes and what they appertain to based on
> syntactic location). Sometimes, this will lead to good behavior (such as
> semantic checks and when doing AST matching over const-qualified types) and
> sometimes it will lead to confusing behavior (pretty-printing the code is
> going to stick const qualifers where none are written, diagnostics will talk
> about const qualifiers that aren't written in the source, etc).
>
> Also, doesn't this cause ABI issues? If you write the attribute on a
> parameter, the presence or absence of that attribute is now part of the ABI
> for the function call because the parameter type will be mangled as being
> const-qualified. (Perhaps that's more of a feature than a bug -- I imagine
> you want both sides of the ABI to agree whether ARC is enabled or not?)
>
> I'm wondering if this is weird enough that it should be using a keyword
> spelling instead of an attribute spelling? However, I'm not certain if that
> works with `#pragma clang attribute`.
> Thanks for letting me know about that use case; it adds some helpful context.
> However, I don't see why this falls apart -- if you're auditing the code, you
> could add the const qualifiers at that time, couldn't you?
If we're going to require O(n) annotations for each application, then there
isn't really any reason to use the automatic #pragma annotations. I think it
would be nice to be able to throw this around more easily, so you could see
what parts of your program are worth auditing, and what stuff breaks. I don't
think the const annotation is a dealbreaker for me, but I think it makes this
feature easier to use and more consistent.
> Yes, but it's weird behavior for an attribute. The attribute is applied to
> the *declaration* but then it silently modifies the *type* as well, but only
> for that one declaration (which is at odds with the usual rules for
> double-square bracket attributes and what they appertain to based on
> syntactic location). Sometimes, this will lead to good behavior (such as
> semantic checks and when doing AST matching over const-qualified types) and
> sometimes it will lead to confusing behavior (pretty-printing the code is
> going to stick const qualifers where none are written, diagnostics will talk
> about const qualifiers that aren't written in the source, etc).
Well, we would get the bad diagnostics and stuff about const where none was
written with a keyword that implied `const` as well. I don't think that the
standard double square bracket attribute rules really matter here, since we
don't really follow them for clang extensions. I guess it boils down to the
philosophical question: what should an attribute do? I agree that adding const
is a bit strange, but I don't think its beyond the pale.
> Also, doesn't this cause ABI issues? If you write the attribute on a
> parameter, the presence or absence of that attribute is now part of the ABI
> for the function call because the parameter type will be mangled as being
> const-qualified. (Perhaps that's more of a feature than a bug -- I imagine
> you want both sides of the ABI to agree whether ARC is enabled or not?)
Top-level cv qualifiers on parameters are ignored in the mangling, so the
parameter isn't mangled as const qualified, I'll add a testcase for this.
> I'm wondering if this is weird enough that it should be using a keyword
> spelling instead of an attribute spelling? However, I'm not certain if that
> works with #pragma clang attribute.
That's not the end of the world, we could just add a new custom pragma if we
went with a keyword. I don't think we want to put this in the type system,
since I think it only really makes sense to use this with declarations of
parameters and variables. AFAIK an attribute is the only way to uniformly add
something to every kind of declaration that we want to support (objc method
parameter, variable, parameter), so I think it's the best way to expose this.
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
