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

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked 2 inline comments as done.
simon_tatham added a comment.

On the general discomfort with this attribute existing: I'd be happy to lock it down, or mark it as "not recommended" in some way, if that's any help. I don't personally intend any use of it outside a single system header file (namely `arm_mve.h`, which D67161 <https://reviews.llvm.org/D67161> will introduce the initial version of).

A warning along the lines of "don't use this!", automatically suppressed by the usual change of warning settings in system headers, would seem like a perfectly reasonable precaution, for example.



================
Comment at: clang/include/clang/Basic/Attr.td:596
 }
+def ClangBuiltinOverride : Attr {
+  let Spellings = [GCC<"__clang_builtin">];
----------------
aaron.ballman wrote:
> 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?
For my use case, I have no opinion about redeclarations: I expect to declare each affected function exactly once. If you think `InheritableAttr` is a more sensible default choice, I'm fine with that.

Target-specific: I don't have a use case outside the ARM target, so I'd be happy to lock it down that way if you want.


================
Comment at: clang/include/clang/Basic/Attr.td:600
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> No new undocumented attributes, please.
OK. I'd intended to leave it undocumented in order to discourage people from using it in any context //other// than a system header file. But fair enough – perhaps it should be documented even so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67159