[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">];
----------------
Should this be a target-specific attribute as it only has effects on ELF targets?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2834
   }
+  if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr<RetainAttr>()) {
+    RetainAttr *NewAttr = OldAttr->clone(Context);
----------------
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.)


================
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}}
----------------
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).


================
Comment at: clang/test/Sema/attr-retain.c:8
+void foo() {
+  int l __attribute__((retain)); // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+}
----------------
Can you also add a test case showing the attribute does not accept any arguments?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96838/new/

https://reviews.llvm.org/D96838