cplusplus / draft

C++ standards drafts
http://www.open-std.org/jtc1/sc22/wg21/
5.65k stars 746 forks source link

using const_reverse_iterator = reverse_iterator<const_iterator>; #6053

Open notbob-at-tessellation-com opened 1 year ago

notbob-at-tessellation-com commented 1 year ago

Hello,

Today I am making my first posts to this group. Please forgive me if I make mistakes. Thank you.

Here is the context:

Header :

template<class charT, class traits = char_traits> class basic_string_view { public: // types using traits_type = traits; using value_type = charT; using pointer = value_type; using const_pointer = const value_type; using reference = value_type&; using const_reference = const value_type&; using const_iterator = implementation-defined ; // see 24.4.2.2 using iterator = const_iterator;228 using const_reverse_iterator = reverse_iterator; using reverse_iterator = const_reverse_iterator;

I understand that the alias declaration for const_reverse_iterator uses the only declaration of reverse_iterator that is in scope at that moment, which is std::reverse_iterator, but then reverse_iterator is defined again on the next line, which creates an ordering dependency. Wouldn't it be safer and clearer to say:

using const_reverse_iterator = std::reverse_iterator; using reverse_iterator = const_reverse_iterator;

or even:

using const_reverse_iterator = std::reverse_iterator; using reverse_iterator = std::reverse_iterator;

as they end up being the same type anyway, and this would eliminate the ordering issue?

Thank you,

Robert Schwartz

jwakely commented 1 year ago

Yes, we should use std::reverse_iterator here. I don't think we should change the definition of reverse_iterator though, it's clearer to show that it's the same type as const_reverse_iterator by defining it to be exactly that, not by defining them separately to the same type.

notbob-at-tessellation-com commented 1 year ago

Thank you, that makes sense to me.

Robert

On Jan 18, 2023, at 9:45 AM, Jonathan Wakely @.***> wrote:

Yes, we should use std::reverse_iterator here. I don't think we should change the definition of reverse_iterator though, it's clearer to show that it's the same type as const_reverse_iterator by defining it to be exactly that, not by defining them separately to the same type.

— Reply to this email directly, view it on GitHubhttps://github.com/cplusplus/draft/issues/6053#issuecomment-1387188128, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A5KLPJ6AV24O4LEYA7GGZNLWS76YHANCNFSM6AAAAAAT7FGVSQ. You are receiving this because you authored the thread.Message ID: @.***>

Quuxplusone commented 1 year ago

A quick git grep 'reverse_iterator *=' source/ shows that [string.view] is the only section that uses the inconsistent style. Everyone else does the same thing (NB: even set, whose iterators and const_iterators are the same type):

source/containers.tex:    using reverse_iterator       = std::reverse_iterator<iterator>;
source/containers.tex:    using const_reverse_iterator = std::reverse_iterator<const_iterator>;

Except for [span], which does it the other way around:

source/containers.tex:    using reverse_iterator = std::reverse_iterator<iterator>;
source/containers.tex:    using const_reverse_iterator = std::const_iterator<reverse_iterator>;

That looks like a typo, but in fact it was intentional: 32535186b implements @brevzin's P2278R4 "cbegin should always return a constant iterator", which has a whole section admitting that this is confusing but done intentionally so that span.crbegin() and ranges::crbegin(span) have the same type. There's no discussion there of why we care about those types being identical but not, say, the types of vector.crbegin() and ranges::crbegin(vector).