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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Headers/intrin.h:435
 #if defined(__i386__) || defined(__x86_64__)
-static __inline__ void __DEFAULT_FN_ATTRS
-__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n) {
+void __DEFAULT_FN_ATTRS __movsb(unsigned char *__dst,
+                                unsigned char const *__src, size_t __n) {
----------------
The functions with inline definitions should still be `static inline` so that we don't emit them as strong external defintiions.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9672-9673
+      if (unsigned BuiltinID = II->getBuiltinID()) {
+        const auto *LinkageDecl =
+            dyn_cast<LinkageSpecDecl>(NewFD->getDeclContext());
+
----------------
This will give the wrong answer for
```
extern "C" {
namespace X {
void __builtin_foo();
}
}
```
... which does have C language linkage. Instead, please call `FunctionDecl::getLanguageLinkage()`, which knows how to handle these cases.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:9690-9691
+            if (!Error && !BuiltinType.isNull()) {
+              // We want noexcept declarations to match. Create an identical
+              // function type, but remove the exception spec.
+              const FunctionProtoType *Type =
----------------
Please use `ASTContext::hasSameTypeIgnoringExceptionSpec` instead.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:914
+  return II.hadMacroDefinition() || II.isPoisoned() ||
+         II.getObjCOrBuiltinID() || II.hasRevertedTokenIDToIdentifier() ||
          (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&
----------------
We now consider `getObjCOrBuiltinID()` here for the `IsModule` case, where we didn't before. Is that an intentional change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491