This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Don't force a conversion to difference_type in std::advance
ClosedPublic

Authored by ldionne on Jun 8 2020, 1:22 PM.

Details

Summary

The Standard documents the signature of std::advance as

template <class Iter, class Distance>
constexpr void advance(Iter& i, Distance n);

Furthermore, it does not appear to put any restriction on what the type
of Distance should be. While it is understood that it should usually
be std::iterator_traits::difference_type, I couldn't find any wording
that mandates that. Similarly, I couldn't find wording that forces the
distance to be a signed type.

This patch changes std::advance to accept any type in the second argument,
which appears to be what the Standard mandates. I think it would be better
to at least restrict it to integral types, but that's not what the Standard
appears to say.

Diff Detail

Event Timeline

ldionne created this revision.Jun 8 2020, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 1:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

@mclow.lists I'm curious to hear what you think about this patch.

I think it's weird to have a wide open template parameter like this in std::distance, but it appears to be what the Standard wants. And using difference_type leads to warnings when passing an unsigned type when -Wsign-conversion is enabled (which is where this change comes from).

I think it's weird to have a wide open template parameter like this in std::distance, but it appears to be what the Standard wants.

Personally, I think that this was a mistake in the original STL design. But there it is.

I think you need to do the same dance as copy_n does.

And make sure you don't break the advance(bidi, -3). There was an LWG issue about that.

I wouldn't change the __advance definitions. That's just spreading the pain around. Convert from Distance to iterator_traits<...>::difference_type in advance

ldionne updated this revision to Diff 271101.Jun 16 2020, 7:55 AM

Update per Marshall's comments

ldionne accepted this revision as: Restricted Project.Jun 16 2020, 10:47 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 16 2020, 11:00 AM
This revision was automatically updated to reflect the committed changes.