[Bug libstdc++/58403] New: __normal_iterator triggers odr-use

[Bug libstdc++/58403] New: __normal_iterator triggers odr-use

[lichray at gmail dot com]

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

            Bug ID: 58403
           Summary: __normal_iterator triggers odr-use
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: lichray at gmail dot com

Some __normal_iterator's operators take `const difference_type&`, which
triggers odr-use of its argument and results in a linker-error.

Both reverse_iterator and move_iterator take `difference_type`, and
libc++ does it for its __wrap_iter as well.  I think this should be
a recommended choice.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo.carlini at oracle dot com]

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

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Changing this would be of course trivial. I'd like to see one of those
linker-errors though.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[lichray at gmail dot com]

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

--- Comment #2 from Zhihao Yuan <lichray at gmail dot com> ---
Created attachment 30807
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30807&action=edit
test case

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo.carlini at oracle dot com]

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

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2013-09-12
           Assignee|unassigned at gcc dot gnu.org      |paolo.carlini at oracle dot com
   Target Milestone|---                         |4.9.0
     Ever confirmed|0                           |1

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Ok, thanks. Let's do this then.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Although I see no harm in changing the library to take those arguments by
value, the testcase is of course invalid.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[lichray at gmail dot com]

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

--- Comment #5 from Zhihao Yuan <lichray at gmail dot com> ---
(In reply to Jonathan Wakely from comment #4)
> Although I see no harm in changing the library to take those arguments by
> value, the testcase is of course invalid.

Can you elaborate?

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo.carlini at oracle dot com]

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

Paolo Carlini <paolo.carlini at oracle dot com> changed:

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

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Please.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
http://gcc.gnu.org/wiki/VerboseDiagnostics#missing_static_const_definition

You odr-use a static member variable, so it must have a definition.  The linker
error is your fault for not defining the static member, not the library's fault
for using a reference parameter.

If you add the missing definition the program links:

constexpr std::iterator_traits<std::string::iterator>::difference_type A::a;

So I have no objection to changing __normal_iterator to take the parameter by
value, but taking it by reference is not a bug.  It is unspecified whether
"it += A::a" is an odr-use or not.

Your program will also fail to link if you do this:

    std::max(A::a, A::a);

But clearly we're not going to change std::max, that would be non-conforming.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo.carlini at oracle dot com]

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

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Hi,

(In reply to Jonathan Wakely from comment #7)
> http://gcc.gnu.org/wiki/VerboseDiagnostics#missing_static_const_definition
> 
> You odr-use a static member variable, so it must have a definition.

I may be missing some C++11 specifics, but I understand the bug report this
way: since the implementors have the leeway of defining those operators as
taking the arguments by value or be const ref, and since we are essentially
talking about ptrdiff_ts, which don't really want by ref, why using by const
ref unnecessarily which introduces an odr-use? If you like not a proper bug,
but a QoI issue, a rather simple one. Taking also into account, as submitter
correctly noticed, that we have already be value for move_iterator and
reverse_iterator. Does this make sense?

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[lichray at gmail dot com]

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

--- Comment #9 from Zhihao Yuan <lichray at gmail dot com> ---
(In reply to Jonathan Wakely from comment #7)
> [...] It is unspecified whether
> "it += A::a" is an odr-use or not.

Well, it's true, but I guess the primary purpose of making
containers' iterator implementation-defined is to allow
raw pointers instead of to make it harder to use...

I think we have the consensus that this is OK to change,
though.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, I agree it's QoI.

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo at gcc dot gnu.org]

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

--- Comment #11 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> ---
Author: paolo
Date: Thu Sep 12 15:15:34 2013
New Revision: 202531

URL: http://gcc.gnu.org/viewcvs?rev=202531&root=gcc&view=rev
Log:
2013-09-12  Paolo Carlini  <paolo.carlini@oracle.com>

    PR libstdc++/58403
    * include/bits/stl_iterator.h (__normal_iterator<>::operator[],
    operator+=, operator+, operator-=, operator-): Take the argument
    by value.
    * testsuite/24_iterators/normal_iterator/58403.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/24_iterators/normal_iterator/58403.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/stl_iterator.h

[Bug libstdc++/58403] __normal_iterator triggers odr-use

[paolo.carlini at oracle dot com]

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

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Done.