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

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Thanks for the review. All tests still pass, should be good for another round.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:9672-9673
+      if (unsigned BuiltinID = II->getBuiltinID()) {
+        const auto *LinkageDecl =
+            dyn_cast<LinkageSpecDecl>(NewFD->getDeclContext());
+
----------------
rsmith wrote:
> 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.
Good suggestion. This fixes the long-standing FIXME inherited from `getBuiltinID()`. I've added a test for this.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491