aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+ let Spellings = [GCC<"retain">];
----------------
MaskRay wrote:
> aaron.ballman wrote:
> > Should this be a target-specific attribute as it only has effects on ELF
> > targets?
> As I understand it, GCC `retain` is not warned on unsupported targets.
>
> Regardless of GCC's choice, I think not having a `warning: unknown attribute
> 'retain' ignored [-Wunknown-attributes]` diagnostic makes sense. `retain`
> will be usually used together with `used`. In Clang, `used` already has
> "retain" semantics on macOS and Windows (I don't know what they do on GCC;
> GCC folks want orthogonality for ELF and I agree). If `retain` is silently
> ignored on macOS and Windows, then users don't need to condition compile for
> different targets.
The other side of that is a user who writes only `retain` and expects it to do
something when it's actually being silently ignored. While they may usually use
it in conjunction with `used`, I'm more worried about the situation where the
user is possibly confused.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:2834
}
+ if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr<RetainAttr>()) {
+ RetainAttr *NewAttr = OldAttr->clone(Context);
----------------
MaskRay wrote:
> aaron.ballman wrote:
> > I realize you're following the same pattern as the used attribute, but..
> > why is this code even necessary? The attributes are already marked as being
> > inherited in Attr.td, so I'd not expect to need either of these. (If you
> > don't know the answer off the top of your head, that's fine -- I can
> > investigate on my own.)
> Without `setInherited`, `attr-decl-after-definition.c` will fail. (I don't
> know why..) So I will keep the code but add a test case using the variable
> `tentative`.
Thanks, if I get the chance, I'll try to look into why this code is necessary
(and make adjustments if needed).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96838/new/
https://reviews.llvm.org/D96838
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
