MaskRay added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+ let Spellings = [GCC<"retain">];
----------------
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.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:2834
}
+ if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr<RetainAttr>()) {
+ RetainAttr *NewAttr = OldAttr->clone(Context);
----------------
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`.
================
Comment at: clang/test/CodeGen/attr-retain.c:21
+int g1 __attribute__((retain));
+__attribute__((retain)) static int g2;
+__attribute__((used,retain)) static int g3;
----------------
phosek wrote:
> Would it be possible to also include negative check for `g2`?
```
// CHECK-NEXT: @g1 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
// CHECK-NEXT: @g3 = internal global i32 {{.*}} !retain ![[#EMPTY]]
```
excludes accidental g2.
================
Comment at: clang/test/Sema/attr-retain.c:3
+
+extern char test1[] __attribute__((retain)); // expected-warning {{'retain'
attribute ignored}}
+extern const char test2[] __attribute__((retain)); // expected-warning
{{'retain' attribute ignored}}
----------------
aaron.ballman wrote:
> Oof, this is not a particularly friendly diagnostic (there's no mention of
> *why* the attribute is ignored, so it's hard to know how to respond to the
> diagnostic as a user).
I created the test based on the existing attr-used.c. Let me figure out how to
make the diagnostic friendlier...
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
