[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Raul Tambre via Phabricator via cfe-commits]

tambre marked an inline comment as done.
tambre added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:3169-3175
   } else {
-    if (!getIdentifier())
+    const auto *Attr = getAttr<BuiltinAttr>();
+
+    if (!Attr)
       return 0;
 
+    BuiltinID = Attr->getID();
----------------
aaron.ballman wrote:
> I think this is a bit more clear:
> ```
> } else if (const auto *A = getAttr<BuiltinAttr>()) {
>   BuiltinID = A->getID();
> }
> ```
> and initialize `BuiltinID` to zero above.
Done.


================
Comment at: clang/test/CodeGen/callback_pthread_create.c:17
 
+// FIXME: How to do builtin handling for this?
 int pthread_create(pthread_t *, const pthread_attr_t *,
----------------
As many others prior to this patch, `pthread_create` was recognized as a builtin due to its name and thus had attributes applied.
Unlike others however, `pthread_create` is the only builtin in `Builtins.def` that doesn't have its arguments specified. Doing that would require implementing support for function pointers in the builtin database and adding yet another special case for `pthread_t` and `pthread_attr_t`.
That'd be quite a bit of work, which I'm not interested in doing.

How about simply removing the hack that is the `pthread_create` builtin entry and disabling/removing this test?


================
Comment at: clang/test/Sema/implicit-builtin-decl.c:64
-struct __jmp_buf_tag {};
-void sigsetjmp(struct __jmp_buf_tag[1], int); // expected-warning{{declaration of built-in function 'sigsetjmp' requires the declaration of the 'jmp_buf' type, commonly provided in the header <setjmp.h>.}}
 
----------------
aaron.ballman wrote:
> It looks like we're losing test coverage with this change?
Indeed. I've reverted this change and changed the test to instead not test for it being recognized as a builtin, since it shouldn't be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491