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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

@rsmith This is a big enough architectural change that I'd appreciate your sign-off on the basic approach.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3759
   unsigned BuiltinID;
-  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
+  bool RevertedBuiltin{Old->getIdentifier()->hasRevertedBuiltin()};
+  if (Old->isImplicit() &&
----------------
rjmccall wrote:
> The old code didn't check this eagerly.  The `Old->isImplicit()` check is unlikely to fire; please make sure that we don't do any extra work unless it does.
Since BuiltinAttr is inherited, we should keep the isImplicit check, because we only want to use a special diagnostic when "redeclaring" the implicit builtin declaration.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:8880
+      }
+    }
+
----------------
Hmm.  I'm concerned about not doing any sort of semantic compatibility check here before we assign the function special semantics.  Have we really never done those in C++?

If not, I wonder if we can reasonably make an implicit declaration but just make it hidden from lookup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491