aaron.ballman added inline comments.
================
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());
----------------
erik.pilkington wrote:
> 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.
> Well, we would get the bad diagnostics and stuff about const where none was
> written with a keyword that implied const as well.
True, but at the same time, the keyword is a stronger signal to the user that
this is something special that behaves strangely, too.
> 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.
We typically do follow the appertainment rules for extensions and this feature
kind of muddies that because the attribute appertains to both the declaration
and the type in a way. But, as you say, this is an extension and we can be
weird (on reflection, there is some precedent here too -- calling conventions
come to mind as they're sort of type attributes and sort of declaration
attributes).
> 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.
That's a compelling point, thank you for pointing that out!
> ...so I think it's the best way to expose this.
Okay, I'll hold my nose on the const qualifier then; there's enough reasons to
do it, and "ewwwww" isn't a strong enough reason not to do it.
================
Comment at: clang/test/SemaObjC/externally-retained.m:2
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0
-fobjc-runtime=macosx-10.13.0 -fblocks -fobjc-arc %s -verify
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.0
-fobjc-runtime=macosx-10.13.0 -fblocks -fobjc-arc -xobjective-c++ %s -verify
+
----------------
Can you add a test that runs with ARC disabled to test out the `LangOpts`
behavior from Attr.td? (Can be done in a separate test file if you want, too.)
================
Comment at: clang/test/SemaObjC/externally-retained.m:10
+void test1() {
+ EXT_RET int a; // expected-warning{{'objc_externally_retained' can only be
applied to}}
+ EXT_RET __weak ObjCTy *b; // expected-warning{{'objc_externally_retained'
can only be applied to}}
----------------
Can you add a test that demonstrates the attribute accepts no args?
================
Comment at: clang/test/SemaObjC/externally-retained.m:14
+
+ EXT_RET int (^d)() = ^{return 0;};
+ EXT_RET ObjCTy *e = 0;
----------------
Should this be useful for function pointer parameters as well? e.g.,
```
typedef void (*fp)(EXT_RET __strong ObjCTy *);
void f(__strong ObjCTy *);
void g(EXT_RET ObjCTy *Ptr) {
fp Fn = f; // Good idea? Bad idea?
Fn(Ptr); // Which behavior "wins" in this call?
}
```
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
