[libcxx-commits] [PATCH] D127953: [libc++] Mark standard-mandated includes as such

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

In D127953#3590508 <https://reviews.llvm.org/D127953#3590508>, @saugustine wrote:

> I can't speak to the testing issues--perhaps it would be good to add them in a subsequent change. But getting correct code working again would be really good, and what is here addresses that.

Agreed it was not my intention to block this review until there are tests. But it's good to think about how we can prevent this from happening in the future.

In D127953#3590246 <https://reviews.llvm.org/D127953#3590246>, @philnik wrote:

> @jloser @Mordante Do you have suggestions for how such a test would look like? I don't think it's feasible to check for every symbol that should be provided for each header and checking that header X is included in Y sounds a bit fragile to me.

I don't think we need to test everything. But for initializer list just that an `std::initializer<int>` is valid, something similar for `operator<=>`. For the containers we can probably craft something that creates a `container<T>` and validates whether `std::size`, `std::ssize` etc. works. I think these can be compile tests without looking at the results.

There's one nice to have comment, otherwise LGTM! Thanks for the fix and improvement!



================
Comment at: libcxx/include/array:124
 #include <type_traits>
 #include <version>
 
----------------
philnik wrote:
> Mordante wrote:
> > Should we add `<version>` to the mandatory list too? It provides `__cpp_lib_array_constexpr` which this header must provide.
> I don't think we want to add `<version>` because it provides more than by the standard required. We might want to only add the macros needed for each header in them, like libstdc++ and the MSVC STL do. (Although that's not very high on my priority list)
I still think it would be nice, but I don't feel to strong about it. Especially since the CI validates whether the required includes are available.


================
Comment at: libcxx/include/array:127
+// standard-mandated includes
+#include <__iterator/access.h>
+#include <__iterator/data.h>
----------------
philnik wrote:
> Mordante wrote:
> > Since these aren't obvious how do you feel about adding a comment like  `// [iterator.range]/1` to give a hint why it's needed. 
> > 
> > `<compare>` and `<initializer_list>` are obvious, so I don't feel to strong about these.
> I think it's a good idea to say which section mandates them. Added comments for all headers.
Thanks!


================
Comment at: libcxx/include/coroutine:50
+// standard-mandated includes
+#include <compare>
+
----------------
I think it would be nice for these too, but they are not as "hidden" as the partial iterator requirements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127953