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:
> > 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`.
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
