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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
