[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

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