[Bug libstdc++/45549] New: merge is_iterator into iterator_traits

[Bug libstdc++/45549] New: merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

Hello,

after bug 40497, an is_iterator machinery was added to restrict the C++0x
prototypes of std::next and std::prev.

An alternative solution seems to be to use this same machinery in the
definition of iterator_traits so that when a class T is not a pointer and does
not provide iterator_category (and possibly the 4 other types),
iterator_traits<T> is empty (instead of containing 5 broken typedefs). This way
iterator_traits can work with sfinae. It allows reverting the prototypes of
next and prev to the standard ones. It is also more or less what Howard
recommends (see
http://groups.google.com/group/comp.lang.c++/browse_thread/thread/54b1537c08997e10/5006b7e443ef0c62
).

Now, this would incidentally let code with user-defined distance functions work
as people expect, which has been argued against strongly in the past by Gaby
(but then his arguments would likely forbid the resolution of bug 40497).

What is your current opinion on this?


-- 
           Summary: merge is_iterator into iterator_traits
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: marc dot glisse at normalesup dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #1 from paolo dot carlini at oracle dot com  2010-09-05 22:22 -------
(In reply to comment #0)
> An alternative solution seems to be to use this same machinery in the
> definition of iterator_traits so that when a class T is not a pointer and does
> not provide iterator_category (and possibly the 4 other types),
> iterator_traits<T> is empty (instead of containing 5 broken typedefs).

I don't think I can work on this very soon, and I also believe that
__is_iterator can be useful anyway, maybe Jon has more tho say (or do) in this
area... Anyway, are you sure that, given the current wording in C++0x, such
iterator_traits is strictly conforming?


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely dot gcc at gmail dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

------- Comment #2 from marc dot glisse at normalesup dot org  2010-09-06 07:12 -------
(In reply to comment #1)
> __is_iterator can be useful anyway,

Of course, they should use the same helper classes but they can coexist,
although the 2 current uses of is_iterator would disappear. I was personally in
favor of having is_iterator in the standard...

> Anyway, are you sure that, given the current wording in C++0x, such
> iterator_traits is strictly conforming?

Howard seems to think so. The first paragraph of [iterator.traits] is good, the
second one not so good (but it could be argued that the definition is provided
for exposition, that it doesn't have to be copied verbatim to every
implementation). It doesn't seem less conforming than what is used for
next/prev.

But then in some sense it is an extension, as it lets not strictly conforming
code work (but doesn't break any conforming code). Which is why I am asking
about an opinion on a possible enhancement, not calling it a bug.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #3 from paolo dot carlini at oracle dot com  2010-09-06 09:42 -------
(In reply to comment #2)
> It doesn't seem less conforming than what is used for
> next/prev.

Well, I think we are comparing two changes of very different impact and size.
In the case of next / prev we have two functions, completely new in C++0x,
getting a bit of constraining in the return type, no additional defaulted
template parameters, only constraining in the return type: I would argue tha,
in general, the way we are living the post-concepts era, this is more or less
something the user looking inside headers of C++ library implementations is
going to find in *many* more places than those where the Standard explicitly
talks about "does not participate to overload resolution". I can also add that
this very thing makes me a little nervous, but I didn't raise the issue
explicitly anywhere, thus... Anyway, in the other case, we are talking about
changing a fundamental building block of the library. Certainly we would do
that only in C++0x mode, agreed, still we are diverging more from C++03 in an
area where the Standard is *not* diverging at all: as far as I can see, either
we could use a defaulted template parameter with the enable_if on __is_iterator
for the default; or we could create a small hierarchy, without enable_if. This
is not something I would deliver for C++03 too, after so many years with a
straightforward implementation, definitely not. Do you have in mind a simpler
way to implement the "smart" iterator_traits?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

------- Comment #4 from marc dot glisse at normalesup dot org  2010-09-06 11:01 -------
(In reply to comment #3)
> Well, I think we are comparing two changes of very different impact and size.

You are right.

> I would argue tha,
> in general, the way we are living the post-concepts era, this is more or less
> something the user looking inside headers of C++ library implementations is
> going to find in *many* more places than those where the Standard explicitly
> talks about "does not participate to overload resolution". I can also add that
> this very thing makes me a little nervous, but I didn't raise the issue
> explicitly anywhere, thus...

I completely agree here. After the removal of concepts, the library is in need
of more concept-related work in the standard, it shouldn't be up to the
implementers.

> Anyway, in the other case, we are talking about
> changing a fundamental building block of the library. Certainly we would do
> that only in C++0x mode, agreed, still we are diverging more from C++03 in an
> area where the Standard is *not* diverging at all: as far as I can see, either
> we could use a defaulted template parameter with the enable_if on __is_iterator
> for the default; or we could create a small hierarchy, without enable_if. This
> is not something I would deliver for C++03 too, after so many years with a
> straightforward implementation, definitely not. 

Ok. It seemed safe enough to me (especially since some other implementations do
it), so I thought I should ask.

> Do you have in mind a simpler
> way to implement the "smart" iterator_traits?

No, I was going with the small hierarchy (ie keep the partial specializations
for pointers, and have the generic implementation derive from helper<Iter,
has_iterator_category<Iter>::value> where helper is empty by default and has a
partial specialization for T,true that contains 5 typedefs). It looked like the
safest option.

Feel free to close the bug if you think it is a bad idea.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #5 from paolo dot carlini at oracle dot com  2010-09-06 11:42 -------
Nice that we agree on many points. Anyway, my plan would be (I cannot resist ;)
preparing a small prototype, using the hierarchy, attach it here, and wait for
Jon' opinion. Then we can make the final decision...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

------- Comment #6 from marc dot glisse at normalesup dot org  2010-09-06 12:21 -------
(In reply to comment #5)
> preparing a small prototype, using the hierarchy, attach it here

Just to make sure, does that mean you are writing the prototype, or do you want
me to? (my employer started the assignment papers in 2009, but the FSF hasn't
had time yet)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #7 from paolo dot carlini at oracle dot com  2010-09-06 12:32 -------
Me, me ;) But, to be clear, your help here and elsewhere would be more than
welcome. If there is something I can do about the paperwork, just let me know!


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #8 from paolo dot carlini at oracle dot com  2010-09-06 17:08 -------
Created an attachment (id=21713)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21713&action=view)
Draft patch, tested x86_64-linux


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

------- Comment #9 from marc dot glisse at normalesup dot org  2010-09-06 17:48 -------
(In reply to comment #8)
> Draft patch, tested x86_64-linux

Nice. Just to confirm, that's indeed what I had in mind, except that I was
going to rename __is_iterator_helper to __has_iterator_category and move
"||is_pointer" from the helper to __is_iterator (and then use
__has_iterator_category for __iterator_traits). But it is equivalent.

Now to the hardest part: deciding whether to apply it ;-)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #10 from paolo dot carlini at oracle dot com  2010-09-06 20:36 -------
Created an attachment (id=21716)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21716&action=view)
The aforementioned variant, again tested x86_64-linux


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[marc dot glisse at normalesup dot org]

------- Comment #11 from marc dot glisse at normalesup dot org  2010-09-06 20:48 -------
(In reply to comment #10)
> The aforementioned variant, again tested x86_64-linux

Wow, cool!
Sorry, I really didn't mean to give you more work...


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #12 from paolo dot carlini at oracle dot com  2010-09-07 14:59 -------
I think we should apply it and see how it goes. I'm thinking that after all we
are not risking much: the class is empty anyway (in terms of ABI) and we are
not risking rejecting valid iterators, only the other way around.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo at gcc dot gnu dot org]

------- Comment #13 from paolo at gcc dot gnu dot org  2010-09-07 22:31 -------
Subject: Bug 45549

Author: paolo
Date: Tue Sep  7 22:30:33 2010
New Revision: 163977

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163977
Log:
2010-09-07  Paolo Carlini  <paolo.carlini@oracle.com>
            Marc Glisse  <marc.glisse@normalesup.org>

        PR libstdc++/45549
        * include/bits/cpp_type_traits.h (__is_iterator_helper): Rename to
        __has_iterator_category.
        (__is_iterator): Adjust.
        * include/bits/stl_iterator_base_types.h (__iterator_traits): Add
        in C++0x mode, use the latter.
        (iterator_traits): In C++0x mode, derive from the latter.
        * include/bits/stl_iterator_base_funcs.h (next, prev): Remove
        enable_if on the return type.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/cpp_type_traits.h
    trunk/libstdc++-v3/include/bits/stl_iterator_base_funcs.h
    trunk/libstdc++-v3/include/bits/stl_iterator_base_types.h


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549

[Bug libstdc++/45549] merge is_iterator into iterator_traits

[paolo dot carlini at oracle dot com]

------- Comment #14 from paolo dot carlini at oracle dot com  2010-09-07 22:32 -------
Done.


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.6.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45549