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

Thu Sep 3 09:12:08 PDT 2020

tambre added a comment.

In D77491#2254065 <https://reviews.llvm.org/D77491#2254065>, @rjmccall wrote:

> The builtins with custom type-checking are all true intrinsics like `__builtin_operator_new` and so on.  They really can't be validly declared by the user program.  The thing that seems most likely to avoid random compiler crashes would be to either forbid explicit declarations of them or treat those as no longer being builtins.  If we need to maintain compatibility with people making custom declarations, we would need to always treat them as builtins and run the risk of crashing if someone declares one with a bad signature.  But I don't think it's unfair of us to break those people; that is seriously not reasonable user behavior.
>
> It's possible that custom declarations are people trying to create compatibility shims for compilers that don't provide these as builtins.  Those people should be guarding their custom declarations, preferably with `__has_builtin`.

I fully agree.

However, I believe you forget to account for the example that I brought up.
In particular, MSVC's header `vadefs.h` includes a declaration of `__va_start()`, which would cause almost any code including standard headers to fail to compile on Windows.
This issue isn't isolated to some old MSVC version, as the declaration is still there in the latest Visual Studio Preview version 14.28.29213.

How about turning it into an error only on non-Windows?
Though keeping that as a followup might be even better, as this will probably be merged into 11.0.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491