[PATCH] D157793: [Headers] Add missing __need_ macros to stdarg.h

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D157793#4585685 <https://reviews.llvm.org/D157793#4585685>, @iana wrote:

> In D157793#4585268 <https://reviews.llvm.org/D157793#4585268>, @aaron.ballman wrote:
>
>> In D157793#4583698 <https://reviews.llvm.org/D157793#4583698>, @iana wrote:
>>
>>> In D157793#4583663 <https://reviews.llvm.org/D157793#4583663>, @MaskRay wrote:
>>>
>>>>> Apple needs a __need_ macro for va_list. Add one, and also ones for va_start/va_arg/va_end, __va_copy, va_copy.
>>>>
>>>> Do you have a link to the specification or the source that this is needed?
>>>
>>> We need is so that our <sys/_types/_va_list.h> doesn't have to redeclare va_list and doesn't pull in all of stdarg.h either. As far as I know there's no specification for this (or stddef.h) being include-able in pieces.
>>
>> I'm not opposed, but this adds significant complexity to support a questionable use case -- the C standard library headers are not intended to be partially included, so if the plan is to use these `__need` macros in all of the headers we provide, I think that should go through an RFC process to be sure we want to maintain the added complexity. (Also, who is "we" in this case?)
>
> We is Apple.

Thank you!

> We have a <sys/_types/_va_list.h> header that is generating redeclaration warnings in modules mode. I can't just include <stdarg.h> instead since it would change the semantics of <sys/_types/_va_list.h>. stdarg.h and stddef.h appear to be the only clang headers that Apple needs to use in pieces though, I don't think we should add this complexity to any other headers for consistency or "just because".

Thank you for the explanation; if it's just these two files, I think that's pretty reasonable. I was worried it was also going to spill into limits.h, stdint.h, and other freestanding headers.



================
Comment at: clang/test/Headers/stdarg.c:2
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify=c89 -Werror=implicit-function-declaration -std=c89 %t/stdarg0.c
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify=c99 -Werror=implicit-function-declaration -std=c99 %t/stdarg0.c
----------------
I think the triples can be removed.


================
Comment at: clang/test/Headers/stdarg.c:11
+static void f(int p, ...) {
+    __gnuc_va_list g; // c89-error{{undeclared}} c99-error{{undeclared}}
+    va_list v; // c89-error{{undeclared}} c99-error{{undeclared}}
----------------
You should spell out the first instance of the diagnostic


================
Comment at: clang/test/Headers/stdarg.c:34
+    __va_copy(g, v);
+    va_copy(g, v); // c89-error{{implicit}} c89-note{{va_copy}} c99-no-diagnostics
+}
----------------
You should spell out these diagnostics, and I think `c99-no-diagnostics` should be placed up by the RUN lines so it's more obvious that we expect no diagnostics in C99 mode.

Actually, this file should perhaps be split into two files as they're testing different things. (I was tripped up seeing no-diagnostics but we have `c99-error` entries above, that's when I realized the split file was being used differently in the RUN lines which is a bit novel.) But I'm not certain I fully understand what your comment means about why we're using split file in the first place, so I might be missing something.


================
Comment at: clang/test/Headers/stdargneeds.c:2
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify -Werror=implicit-function-declaration -std=c89 %t/stdargneeds0.c
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify -Werror=implicit-function-declaration -std=c89 %t/stdargneeds1.c
----------------
Same suggestions in this file regarding triples and spelling out diagnostics, but I think a single file makes sense as this is testing each of the `__needs_whatever` macros the same way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157793