Details
- Reviewers
• Quuxplusone var-const Mordante ldionne - Group Reviewers
Restricted Project - Commits
- rGe06ca312398d: [libc++] Canonicalize the ranges results and their tests
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This patch has several conflicts with D121528, please rebase it.
(I haven't looked to closely at the changes yet.)
libcxx/include/__algorithm/in_fun_result.h | ||
---|---|---|
23 ↗ | (On Diff #415411) | I think this should be #if _LIBCPP_STD_VER > 17 instead. |
libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp | ||
9–10 | I think no-concepts is no longer necessary. | |
10 | Similar to the other patch, I'm concerned there appears to be a discrepancy between the guard used in the headers and the one used in the tests. | |
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp | ||
58 | Question: why is it not possible to use _LIBCPP_NO_UNIQUE_ADDRESS? | |
64 | Can you add a comment to explain why min_max_result has a significantly different test (I presume because it's the only class where both member variables have the same type)? |
- Rebased
- Address comments
Thanks for the review @var-const! I'll update D121436 once this PR is landed. Then it should be a simple matter of applying the changes from this patch.
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp | ||
---|---|---|
58 | It's possible, but I think @Mordante wouldn't be very happy about it. In general we want to avoid using the libc++ macros in the test suite, and using the two attributes is completely portable (other than having warnings). |
libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp | ||
---|---|---|
40 | Question: is it okay to remove the tests for rvalue references? | |
88 | I presume you find this test overkill, but just want to make sure. | |
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp | ||
58 | Can you add a brief comment explaining that using _LIBCPP_NO_UNIQUE_ADDRESS is undesirable here because of <...> so that no one gets the urge to refactor this in the future? |
libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp | ||
---|---|---|
58 | I indeed dislike _LIBCPP_NO_UNIQUE_ADDRESS in the tests. Our tests in std should (in theory) work with all Standard library implementations. Using _LIBCPP specific macros make these tests less portable. I know MSVC STL uses our tests, so it's not a purely hypothetical. However I think it makes sense to make a specific test macro for this case, for example TEST_UNIQUE_ADDRESS That macro can then hide we need different attributes for MSVC and Clang. Something along the lines #if TEST_STD_VER >= 17 # if defined(_WIN32) // This might require tuning, not sure what Clang on Windows does. # define TEST_UNIQUE_ADDRESS [[msvc::no_unique_address]] # else # define TEST_UNIQUE_ADDRESS [[no_unique_address]] # endif #else # define TEST_UNIQUE_ADDRESS #endif |
Nice cleanup! LGTM after improving the commit message. Since @var-const had some remarks I leave the final approval to him.
libcxx/include/__algorithm/in_in_out_result.h | ||
---|---|---|
23 | I'm not entirely up-to-date to what we do and don't enable from ranges. So I assume this is intentionally, but please add it to the commit message. |
There's a couple of things I don't understand with this patch. Why do we remove #ifndef _LIBCPP_HAS_NO_INCOMPLETE_RANGES? I think a description of what this commit does would be useful, since it looks like there might have been some discussion leading to this patch that I missed.
LGTM with CI passing and with all the result types being disabled when HAS_NO_INCOMPLTE_RANGES. You'll also need to add UNSUPPORTED: libcpp-has-no-incomplete-ranges to some tests.
libcxx/include/__algorithm/in_found_result.h | ||
---|---|---|
21 | Would it be possible to instead mark *all* the result types with HAS_NO_INCOMPLETE_RANGES? |
libcxx/include/__algorithm/in_out_result.h | ||
---|---|---|
27 | IIRC, we ended up settling on something like _InIter/_OutIter, right? |
libcxx/include/__algorithm/in_out_result.h | ||
---|---|---|
27 | Yes, I'll make a follow-up to rename all the results-type template arguments and the already-shipped aliases. I think it's easier to make it consistent first and then rename them all together in an NFC. |
libcxx/include/__algorithm/in_out_result.h | ||
---|---|---|
27 | SGTM. |
Would it be possible to instead mark *all* the result types with HAS_NO_INCOMPLETE_RANGES?