[PATCH] D67159: [clang] New __attribute__((__clang_builtin)).

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I share in the discomfort expressed for this attribute, but I don't have a better solution in mind just yet, so I'm giving some other review feedback in the meantime.



================
Comment at: clang/include/clang/Basic/Attr.td:596
 }
+def ClangBuiltinOverride : Attr {
+  let Spellings = [GCC<"__clang_builtin">];
----------------
Do you expect this attribute to be inherited on redeclarations? I suspect this should be an `InheritableAttr`.

Also, add a newline above it for visual separation, please.

Finally, should this be a target-specific attribute so that it's only available for your target, or do you expect this attribute to be used on all target architectures?


================
Comment at: clang/include/clang/Basic/Attr.td:597
+def ClangBuiltinOverride : Attr {
+  let Spellings = [GCC<"__clang_builtin">];
+  let Args = [IdentifierArgument<"BuiltinName">];
----------------
This should not use the `GCC` spelling as it's not a GCC attribute. It should likely be a `Clang` spelling instead.


================
Comment at: clang/include/clang/Basic/Attr.td:600
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [Undocumented];
+}
----------------
No new undocumented attributes, please.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7442
+  case ParsedAttr::AT_ClangBuiltinOverride:
+    handleClangBuiltinOverrideAttribute(S, D, AL);
+    break;
----------------
You should be able to call `handleSimpleAttribute<ClangBuiltinOverrideAttr>()` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67159