Details
- Reviewers
ldionne Mordante var-const saugustine - Group Reviewers
Restricted Project - Commits
- rGdb1978b67431: [libc++] Mark standard-mandated includes as such
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This seems reasonable to me. Do you think it's worth adding a test for this behavior to avoid regression in the future, or do you think it's overkill?
Thanks for working on this!
I would like that. Maybe we can make automatically generated tests for some parts, like we already do for the feature-test macros.
libcxx/include/array | ||
---|---|---|
124 | Should we add <version> to the mandatory list too? It provides __cpp_lib_array_constexpr which this header must provide. | |
127 | 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. |
Yeah, that's along the lines of what I was thinking for the (proposed) generated test for this sort of thing. I'd be in favor of that direction as it yields long-term value IMO.
It would be good to get this committed, as it fixes a previous break of otherwise correct source code and therefore anything testing against libc++ top of trunk.
libcxx/include/array | ||
---|---|---|
124 | 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) | |
127 | I think it's a good idea to say which section mandates them. Added comments for all headers. |
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.
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!
libcxx/include/array | ||
---|---|---|
124 | 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. | |
127 | Thanks! | |
libcxx/include/coroutine | ||
50 | I think it would be nice for these too, but they are not as "hidden" as the partial iterator requirements. |
Let's land this. I think it's somewhat difficult to test this properly, so I'm fine as-is.
I'm curious about how we handle proposals in the working group.
https://cplusplus.github.io/LWG/issue3378
Suggests that some of the same language applies to the tuple helpers when including array, etc. Some industry-standard conformance tests are checking for the behavior defined in issue 3378.
Usually we wait for an LWG issue to be accepted by the committee before implementing them. An issue might be closed as NAD or get a different resolution as proposed. I'm a bit surprised that a standard conformance test is testing for something not in the WP yet.
Just curious which tool flags this?
I'm not precisely sure what you mean by 'which tool', but here's a reproduction using a cut-down reproduction of the standard test using clang trunk with libc++
https://godbolt.org/z/jYs7Pz8qj
Note that the proposal, adds the following clause to tuple.helper
"In addition to being available via inclusion of the <tuple> header, the entities defined in this subclause [tuple.helper] are available when any of the headers <array> (24.3.2 [array.syn]), <ranges> (26.2 [ranges.syn]), <span> (24.7.2 [span.syn]), or <utility> (22.2.1 [utility.syn]) are included."
I can see why the standards body received this request. Since array, pair, etc. also have tuple_size specializations, it's very similar in reasoning to including iterator helpers for STL containers that support iterators.
We currently have a request for comment out to the company providing the test suite, and don't really require upstream changes as we're capable of excluding the test(s) until such time that the issue is either resolved or rejected. I just wondered if there was a process in place for such things, since I can't imagine my team being the only ones using popular standard conformance test suites.
As an aside, I apologize for being vague about the actual suite. I'm not entirely sure what our license says about providing that information to external parties.
The example you provided is rightly rejected. You use std::pair, which is provided by <utility>. The standard doesn't require <array> to include <utility>. The example also fails without using std::tuple_size:
https://godbolt.org/z/Excc13Gs6
Using the required header instead works:
https://godbolt.org/z/7xYb645Ys
Alternatively if you intended to get the std::tuple_size of std::array that works when only using <array>:
https://godbolt.org/z/ror9n16vG
Note that the proposal, adds the following clause to tuple.helper
I can see why the standards body received this request. Since array, pair, etc. also have tuple_size specializations, it's very similar in reasoning to including iterator helpers for STL containers that support iterators.
I'm not saying the LWG issue isn't valid. I've glossed over it and it seems sensible.
I just explained what the normal libc++ procedure is.
As an aside, I apologize for being vague about the actual suite. I'm not entirely sure what our license says about providing that information to external parties.
I can understand that. But not being able to provide that information makes it harder for me to understand what the real issue is.
Interesting, I didn't catch that; so the test is even more contrived than I had first thought! I wonder if their interpretation is that all of the specializations should be available when any header referenced in the issue is included. That's certainly something that's not very sensible.
You've fairly answered my question on the process, and thank you for the continued discussion as well. I'll wait for the response from the test suite writers and forward their thoughts through discourse or a followup diff if there's anything interesting to discuss more.
Thanks and regards.
Should we add <version> to the mandatory list too? It provides __cpp_lib_array_constexpr which this header must provide.