Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declaration ring_iterator friend operator+=() #90

Closed
martinmoene opened this issue May 5, 2017 · 10 comments
Closed

Declaration ring_iterator friend operator+=() #90

martinmoene opened this issue May 5, 2017 · 10 comments

Comments

@martinmoene
Copy link

Shouldn't the friend declaration of ring_iterator's operator+=() and operator-=() be

template< class R, bool C >
friend ring_iterator<R,C> & operator+=( ring_iterator<R,C> & it, int i ) noexcept;

template< class R, bool C >
friend ring_iterator<R,C> & operator-=( ring_iterator<R,C> & it, int i ) noexcept;

instead of

friend type & operator+=(  type & it, int i ) noexcept;
friend type & operator-=( type & it, int i ) noexcept;

GNU C++ 5.2 warns (-Wnon-template-friend)

@TBBle
Copy link
Contributor

TBBle commented May 5, 2017

Looks like we've stumbled over FAQ: Why do I get linker errors when I use template friends?.

@TBBle
Copy link
Contributor

TBBle commented May 5, 2017

However, I believe the suggested resolution causes every operator+= instantiation to be friends with every ring_iterator instantiation, even if the template arguments don't match. We probably want to use the more-verbose solution from Making New Friends idiom page. (i.e. pre-declare template operator+= and friend it using template type deduction).

@TBBle
Copy link
Contributor

TBBle commented May 5, 2017

Also, already logged as #77 but never apparently acted upon.

@martinmoene
Copy link
Author

Yeah that's the one.

The compile-time (as opposed to link-time) error message (for my incantation of ring_span) with GNU 5.2 was:

prompt>g++ -std=c++17 -O2 -Wall -Wextra -Wno-unused-parameter -o ring-span-lite.t.exe -I../include/nonstd ring-span-lite.t.cpp ring-span.t.cpp   && ring-span-lite.t.exe
In file included from ring-span-lite.t.hpp:12:0,
                 from ring-span-lite.t.cpp:7:
../include/nonstd/ring_span.hpp:518:46: warning: friend declaration 'nonstd::ring_iterator< <template-parameter-1-1>, <anonymous> >::type& nonstd::operator+=(nonstd::ring_iterator< <template-parameter-1-1>, <anonymous> >::type&, int)' declares a non-template function [-Wnon-template-friend]
     friend type& operator+=(type& it, int i) noexcept;
                                              ^
../include/nonstd/ring_span.hpp:518:46: note: (if this is not what you intended, make sure the function template has already been declared and add <> after the function name here)

@martinmoene
Copy link
Author

@TBBle Thanks for your notes.

What are reasons to not declare ring_iterator's member operator+=() and operator-=() non-friend like:

type & operator+=( int i ) noexcept
{
    m_idx += i;
    return *this;
}

and operator+() and operator-() in the sg14 (eventually std) namespace like:

template< class Ring, bool is_const >
ring_iterator<Ring,is_const> operator+( ring_iterator<Ring,is_const> it, int i ) noexcept
{
    it += i;
    return it;
}

@TBBle
Copy link
Contributor

TBBle commented May 6, 2017

I'd defer that question to the paper editors, @hatcat and @Quuxplusone.

Side-note, I've just confirmed this warning on g++ 6.3.0. I intend under #96 to try and get better test coverage in-place so that things like this can't easily slip past.

@TBBle
Copy link
Contributor

TBBle commented May 6, 2017

Yeah, the free-function operators should be in the sg14 namespace. Almost certainly an oversight, it breaks the build on VS2017 which can't find them, and hence can't use std::distance and hence std::inner_product from the unit tests.

Edit: Different issue. We're actually missing operator-(ring_iterator<Ring,is_const>,ring_iterator<Ring,is_const>).

@TBBle
Copy link
Contributor

TBBle commented May 6, 2017

Binary Operators in the Stackoverflow C++ Operator mega-answer agrees with @martinmoene's approach:

For the binary arithmetic operators, do not forget to obey the third basic rule operator overloading: If you provide +, also provide +=, if you provide -, do not omit -=, etc. Andrew Koenig is said to have been the first to observe that the compound assignment operators can be used as a base for their non-compound counterparts. That is, operator + is implemented in terms of +=, - is implemented in terms of -= etc.

According to our rules of thumb, + and its companions should be non-members, while their compound assignment counterparts (+= etc.), changing their left argument, should be a member. Here is the exemplary code for += and +, the other binary arithmetic operators should be implemented in the same way:

TBBle added a commit to TBBle/SG14 that referenced this issue May 6, 2017
Resovles WG21-SG14#90, but is simply the most minimal change.

* Inline the bodies of the friend functions operator+= and operator-=
* Add inline friend operator- for a pair of ring_iterator, to support
  std::difference on MSVC
* Put the free functions operator+ and operator- in the sg14 namespace
  so they are visible to ADL
@TBBle
Copy link
Contributor

TBBle commented May 6, 2017

My current hack-tree is https://github.com/TBBle/SG14/tree/SG14-build-fix-hacks, which when merged with #95, gets everything building on the platforms I happen to be testing on (four Windows x64 compilers). I simply took the Make New Friends idiom in order to get it to compile. The above-described approach is better.

Quuxplusone added a commit to Quuxplusone/SG14 that referenced this issue May 9, 2017
- Add `#pragma once`.
- Add a non-member ADL swap() for rings.
- Fix up the namespace of operator+= and operator-= for ring iterators.
- Fix up (it + n) and (it - n) for ring iterators.
- Add (it - it) for ring iterators.
- Add the implicit conversion from iterator to const_iterator.
- `#if 0` the broken tests for unimplemented static_ring and dynamic_ring.

Fixes WG21-SG14#77, WG21-SG14#90.
TBBle added a commit to TBBle/SG14 that referenced this issue May 10, 2017
Resovles WG21-SG14#90, but is simply the most minimal change.

* Inline the bodies of the friend functions operator+= and operator-=
* Add inline friend operator- for a pair of ring_iterator, to support
  std::difference on MSVC
* Put the free functions operator+ and operator- in the sg14 namespace
  so they are visible to ADL
Quuxplusone added a commit to Quuxplusone/SG14 that referenced this issue May 12, 2017
- Add `#pragma once`.
- Add a non-member ADL swap() for rings.
- Fix up the namespace of operator+= and operator-= for ring iterators.
- Fix up (it + n) and (it - n) for ring iterators.
- Add (it - it) for ring iterators.
- Fix a typo in (a >= b) for ring iterators, and make (a <= b) == !(b > a).
- Add the implicit conversion from iterator to const_iterator.
- `#if 0` the broken tests for unimplemented static_ring and dynamic_ring.

Fixes WG21-SG14#77, WG21-SG14#90, WG21-SG14#93, WG21-SG14#101.
Quuxplusone added a commit to Quuxplusone/SG14 that referenced this issue May 12, 2017
- Add `#pragma once`.
- Add a non-member ADL swap() for rings.
- Fix up the namespace of operator+= and operator-= for ring iterators.
- Fix up (it + n) and (it - n) for ring iterators.
- Add (it - it) for ring iterators.
- Fix a typo in (a >= b) for ring iterators, and make (a <= b) == !(b > a).
- Add the implicit conversion from iterator to const_iterator.
- `#if 0` the broken tests for unimplemented static_ring and dynamic_ring.

Fixes WG21-SG14#77, WG21-SG14#90, WG21-SG14#93, WG21-SG14#101.
Quuxplusone added a commit to Quuxplusone/SG14 that referenced this issue May 12, 2017
- Add `#pragma once`.
- Add a non-member ADL swap() for rings.
- Fix up the namespace of operator+= and operator-= for ring iterators.
- Fix up (it + n) and (it - n) for ring iterators.
- Add (it - it) for ring iterators.
- Fix a typo in (a >= b) for ring iterators, and make (a <= b) == !(b > a).
- Add the implicit conversion from iterator to const_iterator.
- `#if 0` the broken tests for unimplemented static_ring and dynamic_ring.

Fixes WG21-SG14#75, WG21-SG14#77, WG21-SG14#90, WG21-SG14#93, WG21-SG14#101.
Quuxplusone added a commit to Quuxplusone/SG14 that referenced this issue May 12, 2017
- Add `#pragma once`.
- Add a non-member ADL swap() for rings.
- Fix up the namespace of operator+= and operator-= for ring iterators.
- Fix up (it + n) and (it - n) for ring iterators.
- Add (it - it) for ring iterators.
- Fix a typo in (a >= b) for ring iterators, and make (a <= b) == !(b > a).
- Add the implicit conversion from iterator to const_iterator.
- `#if 0` the broken tests for unimplemented static_ring and dynamic_ring.

Fixes WG21-SG14#75, WG21-SG14#77, WG21-SG14#90, WG21-SG14#93, WG21-SG14#101.
@Quuxplusone
Copy link
Contributor

Can this issue be closed as "fixed," at this point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants