[PATCH] D139436: [C2x] Relaxing requirements for va_start

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Headers/stdarg.h:30
+/* C2x does not require the second parameter for va_start. */
+#define va_start(ap, ...) __builtin_va_start(ap, 0)
+#else
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > cor3ntin wrote:
> > > Yeah, i really think there should be some king of diagnostic here.
> > > Maybe `__builtin_va_start` could take an arbitrary number of arguments and diagnose for more that 2 args or if the second arg is not valid?
> > > is it a matter of being compatible with gcc? do we need to?
> > We try pretty hard to keep our builtins compatible with GCC; that makes everyone's lives easier (most especially standard library authors). If we did anything here, it would be to add a new builtin specific for the new variant. However, that won't save us because of the standard's requirement to not *expand* the variadic arguments. We'd need that builtin to be named `va_start` and we'd have to get rid of this macro (otherwise, the arguments passed to the builtin will be expanded as they're lexed). But `va_start` is defined by the standard as being a macro specifically, so we'd still need to make `#if defined(va_start)` work. Ultimately, I don't think it's worth that level of heroics -- instead, I am hoping WG14 will relax the requirement that the arguments not be expanded or evaluated; that gives us options.
> We probably can reuse the same macro name (we'd still have to handle the 0 case though), but if you'd rather be conforming now and revisit the issue when WG14 remove the limitation that parameters are not expanded, I'm fine with it!
For the moment, I'd rather match what GCC is doing (deficiencies and all) until it's clear what actually gets into the final standard. We can revisit once WG14 has said their piece.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7230
+      Val && LangOpts.C2x && *Val == 0)
+    return false;
 
----------------
erichkeane wrote:
> I see below that we still set the return type of hte call to Void.  Do we wnat to keep that line, or at least, move it to the top of this function?
I added test coverage and it seems that setting the return type is not needed (by my understanding, `CreateBuiltin` requires the return type to be passed to it explicitly, and `LazilyCreateBuiltin` uses the builtin type string to specify *at least* the return type information (even if custom type checking happens for the rest of the signature).


================
Comment at: clang/test/C/C2x/n2975.c:49
+
+void foo(int a...); // expected-error {{C requires a comma prior to the ellipsis in a variadic function type}}
----------------
erichkeane wrote:
> erichkeane wrote:
> > .  
> Was going to complain about the error message here, but I'm on the fence whether we should say THIS (the required comma), or something more descriptive, but at least this is better than GCC just failing in normal parsing.
FWIW, this is existing diagnostic wording and behavior. I think it's reasonable, but if we want to do something better, we can fix it in a follow-up.


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

https://reviews.llvm.org/D139436