Page MenuHomePhabricator

[libcxx] adds `iter_difference_t` and `iter_value_t`
ClosedPublic

Authored by cjdb on Apr 4 2021, 11:53 AM.

Details

Summary

Implements parts of:

  • P0896R4 The One Ranges Proposal

Depends on D99855.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 4 2021, 11:53 AM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2021, 11:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
miscco added inline comments.Apr 4 2021, 12:07 PM
libcxx/include/iterator
440

Can we get rid of the __extract_iter_value_t struct alltogether and always fall back to an alias?

template<class _Ip>
requires __is_primary_template<iterator_traits<remove_cvref_t<_Ip>>>::value &&
         __has_member_value_type<indirectly_readable_traits<remove_cvref_t<_Ip>>>
using iter_value_t = typename indirectly_readable_traits<remove_cvref_t<_Ip>>::value_type;

template<class _Ip>
using iter_value_t = typename iterator_traits<remove_cvref_t<_Ip>>::value_type;

That way we would get around all those type instantiations, which are quite costly

447

This should be class

449

The __has_integral_minus Is not obvious should have add a comment?

Mordante added inline comments.Apr 4 2021, 12:17 PM
libcxx/include/iterator
445

For consistency, please add backticks around indirectly_­readable_­traits<R[I]>::value_­type.

454

For consistency, please add backticks around iterator_­traits<R[I]>::value_­type.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.fail.cpp
19 ↗(On Diff #335166)

Why not use a specific offset like expected-errror@-1?

miscco added inline comments.Apr 4 2021, 12:28 PM
libcxx/include/iterator
449

Please disregard, read the other patch

cjdb updated this revision to Diff 335172.Apr 4 2021, 2:03 PM
cjdb marked 7 inline comments as done.

applies feedback from @miscco and @Mordante

libcxx/include/iterator
440

Aliases can't be specialised. Might be worth a compiler patch to elide this if it's deemed too costly?

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.fail.cpp
19 ↗(On Diff #335166)

The error originates from <iterator>, not this file (I originally did expected-error@-1 which is how I learnt about @iterator:* when it failed).

cjdb marked 2 inline comments as not done.Apr 4 2021, 2:11 PM
cjdb added inline comments.
libcxx/include/iterator
449

I can't find your comment anywhere else?

Quuxplusone added inline comments.
libcxx/include/iterator
438–439

Slightly disentangled: so that code doesn't interrupt the flow of an English sentence, and the primary template isn't forward-declared.

I think this is fine; but it does feel weird at a higher level that http://eel.is/c++draft/sequences#general-2 already defines __iter_value_type (as an exposition-only implementation detail related to container deduction guides), and here http://eel.is/c++draft/readable.traits#2 is introducing iter_value_t with a subtly different meaning. libc++ maintainers will have to be careful to pick the right one.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.fail.cpp
32 ↗(On Diff #335172)

FWIW, I think it would make more sense to cast all these tests in terms of what we do expect to be true, instead of relying on compiler error messages.
For example, if we expect that the type std::iter_difference_t<void_subtraction> doesn't exist, we could maybe do

template<class T> auto has_no_difference_type(int) -> std::void_t<std::iter_difference_t<T>> {}
template<class T> bool has_no_difference_type(long) { return true; }

int main(int, char**)
{
    assert(has_no_difference_type<void_subtraction>(42));

    return 0;
}

But this is assuming that iter_difference_t<T> is supposed to be SFINAE-friendly. I don't know if that's actually guaranteed/intended by the Standard.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.pass.cpp
24–45

FWIW, this indentation is insane.

cjdb updated this revision to Diff 335189.Apr 4 2021, 7:07 PM
cjdb marked an inline comment as done and an inline comment as not done.

makes iter_difference_t and iter_value_t SFINAE-friendly and consolidates test files

cjdb marked 2 inline comments as done.Apr 4 2021, 7:09 PM
cjdb added inline comments.
libcxx/include/iterator
438–439

The mixing of standardese with code was intentional, to show which bits of code correspond to which bits of the standard. This idea was taken from <type_traits>.

In future, for something as trivial as this, please request what you're after instead of providing a full-blown suggested edit.

I think this is fine; but it does feel weird at a higher level that http://eel.is/c++draft/sequences#general-2 already defines __iter_value_type (as an exposition-only implementation detail related to container deduction guides), and here http://eel.is/c++draft/readable.traits#2 is introducing iter_value_t with a subtly different meaning. libc++ maintainers will have to be careful to pick the right one.

I think the best thing to do would be to ask LWG to consider renaming iter-value-type to something else (it might even be possible to do this editorially after a mailing discussion), or us just doing so regardless of what LWG thinks, so as to avoid confusion.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.fail.cpp
32 ↗(On Diff #335172)

The wording is SFINAE-friendly, but my implementation here wasn't due to a mistake in translating from cjdb-ranges to libc++.

cjdb updated this revision to Diff 335207.Apr 4 2021, 11:32 PM
cjdb marked an inline comment as done.

rebases to activate CI

Quuxplusone added inline comments.Apr 5 2021, 8:55 AM
libcxx/include/iterator
445

Please write RI, not R[I]. (I know you're trying to reflect that the I in the standard is a subscript letter. However, it's not an array index, and RI functions semantically as a single identifier — there is no separate entity R to be discussed, so we don't need a name for it.)

I don't think you should be instantiating incrementable_traits<RI> in the case that iterator_traits<RI> is the primary template. Please (1) check what the Standard's intent here is, (2) make the fix if needed, and (3) add a test case that detects the fix.

Separately and presumably a moot point in this case: note that conditional_t is compile-time-wasting by design. Anywhere in libc++ that we need its effects and aren't mandated to use conditional_t, we should be using Eric's _If<is-blah, incr-traits, iter-traits> because it instantiates fewer templates.

cjdb updated this revision to Diff 335730.Apr 6 2021, 11:32 PM

replaces conditional_t with _If

cjdb marked an inline comment as done.Apr 6 2021, 11:32 PM
cjdb added inline comments.
libcxx/include/iterator
445

I don't think you should be instantiating incrementable_traits<RI> in the case that iterator_traits<RI> is the primary template. Please (1) check what the Standard's intent here is, (2) make the fix if needed, and (3) add a test case that detects the fix.

It's pretty much left up to us. MSVC's implementation is identical to the conditional_t one, for example, and @CaseyCarter understands the intention better than us all.

tcanens added a subscriber: tcanens.Wed, Apr 7, 7:02 PM
tcanens added inline comments.
libcxx/include/iterator
440

This now has a mix of RI and R[I]

445

I don't think you should be instantiating incrementable_traits<RI> in the case that iterator_traits<RI> is the primary template.

What does this even mean? How do you produce incrementable_­traits<RI>::difference_­type without instantiating incrementable_­traits<RI>?

Quuxplusone added inline comments.Wed, Apr 7, 7:27 PM
libcxx/include/iterator
445

I flipped the logic by accident. I should have said, don't instantiate incrementable_traits<RI> in the case that iterator_traits<RI> is not the primary template, i.e. in the case when we want to just go ahead and trust iterator_traits<RI>::value_type. (And I almost made the exact same error again in typing up this message! Note to self: Here "primary" means "the one we don't want to use.")

cjdb updated this revision to Diff 336696.Sun, Apr 11, 2:36 PM
cjdb marked an inline comment as done.

rebases to activate CI

cjdb updated this revision to Diff 336993.Mon, Apr 12, 4:57 PM

reverts _If to conditional_t to avoid substitution failure problem in future commits

@cjdb wrote:

reverts _If to conditional_t to avoid substitution failure problem in future commits

Is this because of Clang bug https://bugs.llvm.org/show_bug.cgi?id=44833#c5 ? If so, that Clang bug is probably going to block a lot of Ranges implementation until it's fixed, anyway. @ldionne, do you have any guidance here? (1) Should libc++ aim to be the one standard library that works around this Clang bug, or should we just pressure Clang to fix their bug? (2) In the short term, assuming the bug won't be fixed tomorrow, how much should Chris work around Clang bugs in these patches versus just delaying the patches until Clang is ready?

@cjdb wrote:

reverts _If to conditional_t to avoid substitution failure problem in future commits

Is this because of Clang bug https://bugs.llvm.org/show_bug.cgi?id=44833#c5 ? If so, that Clang bug is probably going to block a lot of Ranges implementation until it's fixed, anyway. @ldionne, do you have any guidance here? (1) Should libc++ aim to be the one standard library that works around this Clang bug, or should we just pressure Clang to fix their bug? (2) In the short term, assuming the bug won't be fixed tomorrow, how much should Chris work around Clang bugs in these patches versus just delaying the patches until Clang is ready?

I think we need to push hard on Clang to fix this bug. If working around it for now is feasible then let's do that, but I would not go too far in that direction.

cjdb updated this revision to Diff 337806.Thu, Apr 15, 9:53 AM

rebases to activate CI

ldionne accepted this revision.Thu, Apr 15, 12:32 PM

I think this LGTM. There's some small comments about a missed R[I] -> RI transformation, but I think that's all. LGTM with nitpicks applied.

libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/iter_difference_t.pass.cpp
24–45

Can we please keep comments constructive and polite? It's fine to disagree with the way of formatting things. It's also fine to request changes on a patch due to indentation or some other format-related issue that impacts readability. But please try to root your comment in something objective.

This revision is now accepted and ready to land.Thu, Apr 15, 12:32 PM
cjdb updated this revision to Diff 338400.Sun, Apr 18, 4:55 PM

s/R[I]/RI/

cjdb updated this revision to Diff 338902.Tue, Apr 20, 9:27 AM

rebases to activate CI

This revision was landed with ongoing or failed builds.Tue, Apr 20, 12:02 PM
This revision was automatically updated to reflect the committed changes.
cjdb added a comment.Tue, Apr 20, 12:02 PM

Merged ahead of CI reaching 100% with permission from @ldionne. Apparently only the santatizers, macos, gcc, c++03, and C++20 builds are the most important.