Page MenuHomePhabricator

[libc++][ranges][NFC] Test the specializations of `tuple_{size,element}` for ranges.
ClosedPublic

Authored by var-const on Feb 1 2022, 2:33 AM.

Details

Summary

Also update the synopsis in <ranges> to mention the specializations.

Diff Detail

Event Timeline

var-const created this revision.Feb 1 2022, 2:33 AM
var-const requested review of this revision.Feb 1 2022, 2:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 2:33 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Feb 1 2022, 8:27 AM
Quuxplusone added a subscriber: Quuxplusone.

I believe this is redundant with D108054, which was committed quite a while ago now.
Anyway, specializations and customizations (of swap, hash, get, tuple_size, enable_borrowed_range, is_error_code_enum,...) belong next to the class they appertain to, not in a separate header. So if there's actually something to do here, please move the code into the subrange.h header (at which point it becomes at best a diff against what's there now).

This revision now requires changes to proceed.Feb 1 2022, 8:27 AM
var-const updated this revision to Diff 405036.Feb 1 2022, 12:07 PM
  • remove the specializations to address the feedback;
  • add a link to the patch to the Ranges status page;
  • rebase on main.

I believe this is redundant with D108054, which was committed quite a while ago now.
Anyway, specializations and customizations (of swap, hash, get, tuple_size, enable_borrowed_range, is_error_code_enum,...) belong next to the class they appertain to, not in a separate header. So if there's actually something to do here, please move the code into the subrange.h header (at which point it becomes at best a diff against what's there now).

Oh, thanks a lot, I missed that. The new code is indeed unnecessary.

I think updating the synopsis is still necessary, though. What do you think about the tests? They test the specializations more directly and have the advantage of being compile-time tests, but on the other hand, don't really provide more coverage than the tests from the patch you linked.

var-const retitled this revision from [libc++][ranges] Specialize `tuple_{size,element}` for ranges. to [libc++][ranges][NFC] Test the specializations of `tuple_{size,element}` for ranges..Feb 1 2022, 12:08 PM
var-const edited the summary of this revision. (Show Details)
Quuxplusone requested changes to this revision.Feb 1 2022, 2:01 PM

Requesting changes, in the sense that I think this PR should simply be abandoned.
(If the tests are cleaned up a bit, then sure, they don't seem harmful. But I won't be a positive approver on this; I'll let two other people go on record as claiming it's useful.)

libcxx/include/ranges
257

Please remove the comment.

libcxx/test/std/ranges/tuple_specializations.compile.pass.cpp
20–21 ↗(On Diff #405036)

This comment is unhelpful, because it is unspecified and unobservable whether <ranges> includes <utility>. (I would bet that it does.)
Anyway, if you really really want to make sure, just include nothing but <ranges>, and s/is_same_v/same_as/ below (because same_as is guaranteed to exist in <concepts> which is guaranteed to be included by <ranges>).

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.compile.pass.cpp
11 ↗(On Diff #405036)

I would like us to move away from no-op tests, if possible.

This revision now requires changes to proceed.Feb 1 2022, 2:01 PM
var-const marked an inline comment as done.Feb 1 2022, 2:07 PM

Requesting changes, in the sense that I think this PR should simply be abandoned.
(If the tests are cleaned up a bit, then sure, they don't seem harmful. But I won't be a positive approver on this; I'll let two other people go on record as claiming it's useful.)

Changing the synopsis is still necessary though, right? Also, how would you like to clean up the tests?

libcxx/include/ranges
257

Why?

libcxx/test/std/ranges/tuple_specializations.compile.pass.cpp
20–21 ↗(On Diff #405036)

it is unspecified and unobservable whether <ranges> includes <utility>. (I would bet that it does.)

I still don't think it's a good idea to include it on purpose.

just include nothing but <ranges>, and s/is_same_v/same_as/ below

Done, thanks.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.compile.pass.cpp
11 ↗(On Diff #405036)

Sure. I saw this done in other tests and mimicked that.

var-const marked an inline comment as done.Feb 1 2022, 2:09 PM

they don't seem harmful.

They test a requirement from the Standard that we otherwise test more-or-less accidentally, so it seems like they are actually useful.

var-const updated this revision to Diff 405098.Feb 1 2022, 2:10 PM

Address feedback.

ldionne added a subscriber: ldionne.Feb 1 2022, 3:01 PM

The synopsis updates LGTM. Adding a test for the mandatory inclusions do make sense to me as well, however shouldn't we be doing that via libcxx/utils/generate_header_inclusion_tests.py instead?

Quuxplusone added inline comments.Feb 1 2022, 4:42 PM
libcxx/include/ranges
257

Because (1) we don't add comments for any other headers, and (2) the fact that __ranges/subrange.h includes the API of std::ranges::subrange is obvious. We don't need to say "This makes subrange available, and all its methods, and its get overload, and its tuple_size and tuple_element, and its enable_borrowed_range, and its enable_view, and its..." We don't need to say any of that, because it's obvious from the filename.

var-const updated this revision to Diff 405121.Feb 1 2022, 4:54 PM

Fix a comment in <ranges> regarding includes and rebase on main.

var-const marked 2 inline comments as done.Feb 1 2022, 4:54 PM
var-const added inline comments.
libcxx/include/ranges
257

Ah, thanks for calling it out, the comment is indeed wrong. It should refer to the "general" templates, not the specializations. PTAL.

var-const marked an inline comment as done.Feb 1 2022, 5:03 PM

shouldn't we be doing that via libcxx/utils/generate_header_inclusion_tests.py instead?

The Standard says in [tuple.helper](https://eel.is/c++draft/tuple.helper#6):

In addition to being available via inclusion of the `<tuple>` header, the template is available when any of the headers `<array>`, `<ranges>`, or `<utility>` are included.

(the templates in question are tuple_size and tuple_element)

I don't think generate_header_inclusion_tests.py can do that because it checks for inclusion of headers, not for the availability of particular classes and functions.

The existing tests for the similar requirement in <array> and <utility> do it by instantiating every distinct specialization of array and pair, respectively (test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp and test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.utility.pass.cpp). Now that I think about it, it seems like I should move the new tests under test/std/utilities/tuple/tuple.tuple/tuple.helper, what do you think? Also, would it make sense to instantiate tuple_size and tuple_element for something other than the range specializations (because if I'm reading the requirement correctly, the templates should become generally available, not just the specializations)?

I don't think generate_header_inclusion_tests.py can do that because it checks for inclusion of headers, not for the availability of particular classes and functions.

Oh, now that you show me the standard wording, yes I agree with you 100%.

The existing tests for the similar requirement in <array> and <utility> do it by instantiating every distinct specialization of array and pair, respectively (test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp and test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.utility.pass.cpp). Now that I think about it, it seems like I should move the new tests under test/std/utilities/tuple/tuple.tuple/tuple.helper, what do you think?

Yes, that sounds like the correct thing to do.

Also, would it make sense to instantiate tuple_size and tuple_element for something other than the range specializations (because if I'm reading the requirement correctly, the templates should become generally available, not just the specializations)?

I'm neutral on this. I think we should aim for the simplest test that gives us confidence that we're doing the right thing. We should avoid getting into diminishing returns IMO.

var-const updated this revision to Diff 405404.Feb 2 2022, 1:00 PM

Move the new test under test/std/utilities/tuple/tuple.tuple/tuple.helper so
that it's next to similar tests for <array> and <utility>.

Yes, that sounds like the correct thing to do.

Done.

I'm neutral on this. I think we should aim for the simplest test that gives us confidence that we're doing the right thing. We should avoid getting into diminishing returns IMO.

Ok, in that case, I'll just test the subrange specializations, similar to the existing tests.

var-const updated this revision to Diff 405411.Feb 2 2022, 1:24 PM

Address review feedback:

  • remove a comment in the <ranges> header;
  • add a test case for a sized range with an unsized sentinel.
ldionne accepted this revision.Feb 2 2022, 2:36 PM

LGTM.

I understand @Quuxplusone 's point about this being somewhat redundant, however adding these tests is consistent with what we're doing for the <array> and <utility> tests in the same directory. If we don't find that this adds enough value, we should be removing the tests for <array> and <utility> instead.

However, from my point of view, these three tests do add some value (especially since we like to move stuff around in the headers, and I can definitely imagine we'd break users relying on this guarantee).

var-const added inline comments.Feb 2 2022, 4:05 PM
libcxx/include/ranges
257

(Note: removed the comment per our offline discussion)

This revision was not accepted when it landed; it landed in state Needs Review.Feb 2 2022, 10:58 PM
This revision was automatically updated to reflect the committed changes.

@Quuxplusone Per our offline discussion, my understanding was that you're okay with the patch modulo two comments which I have since addressed. I'll merge based on this understanding -- I can address any additional feedback in a follow-up if necessary.