[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:
> 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.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:7356
+        // in C before C2x. Complain and provide the fix. We do not support
+        // this as an extension in earlier C language modes.
+        if (getLangOpts().C2x)
----------------
erichkeane wrote:
> Is this right?  Isnt this also allowing:
> 
>     foo(int a ...) 
> 
> I don't think we want to do that, right?  GCC diagnoses that as "requires a named argument", not a comma.
Ah good catch! I'll add a test case for that. In fact, I don't think I need to do anything here in C; the existing logic was already correct.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7227
+  bool SecondArgIsElided = false;
+  if (Optional<llvm::APSInt> Val =
+          TheCall->getArg(1)->getIntegerConstantExpr(Context);
----------------
erichkeane wrote:
> This doesn't exactly match the comment, this isn't checking against the literal-0, it is checking against an ICE.
> 
> An updated comment would probably be fine here, and perhaps preferential, in order to allow a paren-expr containing 0.
Ah, fair, we check against `0` explicitly, but we don't care if you got that zero from `0` or from `0 + 0`, etc. I'll update the comment.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7230
+      Val)
+    SecondArgIsElided = LangOpts.C2x && *Val == 0;
 
----------------
erichkeane wrote:
> is there value to not jsut doing an early return here?  There is a lot of work happening below based on the last arg that we can just skip instead.
Hmmm that seems reasonable to me, good call!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139436