[Bug libstdc++/60448] New: swap_ranges does not use ADL correctly

[Bug libstdc++/60448] New: swap_ranges does not use ADL correctly

[public at alisdairm dot net]

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

            Bug ID: 60448
           Summary: swap_ranges does not use ADL correctly
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: public at alisdairm dot net

Created attachment 32286
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32286&action=edit
Demonstrate swap_ranges does not find swap via ADL

The attached program does not correctly use ADL to find the 'swap' member
implemented inside the tagged local class in C++11/C++1y builds.  It is not
expected to compile in C++03 or earlier dialects.

The problem compiles and runs as expected on Clang 3.4 with libc++ in both
C++11 and C++1y modes.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[public at alisdairm dot net]

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

--- Comment #1 from Alisdair Meredith <public at alisdairm dot net> ---
Created attachment 32287
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32287&action=edit
Cleaner tagging scheme for the local class

Simplified the example to more directly expose the local class via ADL without
any funky casts.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Alisdair Meredith from comment #1)
> Created attachment 32287 [details]
> Cleaner tagging scheme for the local class
> 
> Simplified the example to more directly expose the local class via ADL
> without any funky casts.

Clang and G++ both reject this because the swap call is ambiguous

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I don't understand what we can be doing wrong in the library, we just call
std::iter_swap(it1, it2) which calls swap(*it1, *it2) unqualified.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Looks like the difference between libc++ and libstdc++ is whether std::swap()
is declared before std::swap_ranges or not, this reproduces the same behaviour,
with swapper replacing swap_ranges:

namespace tagged
{

template <typename T>
struct Swappable {};

template <typename T>
void swap(Swappable<T> & a, Swappable<T> & b) {
   static_cast<T &>(a).swap(static_cast<T &>(b));
}

} // namespace tagged

namespace std
{
  template<typename T>
    void do_std_swap(T& a, T& b)
    {
      T tmp = static_cast<T&&>(a);
      a = static_cast<T&&>(b);
      b = static_cast<T&&>(tmp);
    }

#ifdef SWAP_FIRST
  template<typename T>
    void swap(T& a, T& b)
    {
      do_std_swap(a, b);
    }
#endif

  template<typename T>
    void swapper(T t)
    {
      swap(*t, *t);
    }

#ifndef SWAP_FIRST
  template<typename T>
    void swap(T& a, T& b)
    {
      do_std_swap(a, b);
    }
#endif

}

int main()
{
  struct local : tagged::Swappable<local> {
    local(int x) : data(x) {}

    local(local const &) = delete;
    local(local      &&) = delete;
    local & operator=(local const &) = delete;
    local & operator=(local      &&) = delete;

    void swap(local & other) {
       auto x = other.data;
       other.data = this->data;
       this->data = x;
    }
  private:
    int data;
  };

  local l{1};
  std::swapper(&l);
}

When SWAP_FIRST is defined G++ and Clang call std::swap, otherwise they find
tagged::swap by ADL.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Slightly different version of that last test:

namespace tagged
{

template <typename T>
struct Swappable {};

#ifndef NO_CUSTOM_SWAP
template <typename T>
void swap(Swappable<T> & a, Swappable<T> & b) {
   static_cast<T &>(a).swap(static_cast<T &>(b));
}
#endif

} // namespace tagged

namespace std
{
#ifdef SWAP_FIRST
  template<typename T>
    void swap(T& a, T& b);
#endif

  template<typename T>
    void swapper(T t)
    {
      swap(*t, *t);
    }

  template<typename T>
    void swap(T& a, T& b)
    {
      T tmp = static_cast<T&&>(a);
      a = static_cast<T&&>(b);
      b = static_cast<T&&>(tmp);
    }
}

int main()
{
  struct local : tagged::Swappable<local> {
    local(int x) : data(x) {}

    local(local const &) = delete;
    local(local      &&) = delete;
    local & operator=(local const &) = delete;
    local & operator=(local      &&) = delete;

    void swap(local & other) {
       auto x = other.data;
       other.data = this->data;
       this->data = x;
    }
  private:
    int data;
  };

  local l{1};
  std::swapper(&l);
}


I must be guessing something wrong about libc++'s std::swap_ranges(), because
if it was declared before std::swap() then it would never work with any type
that doesn't provide its own swap() overload, as demonstrated by compiling the
above with NO_CUSTOM_SWAP defined.

So I don't know exactly what's going on, but I'm not convinced libstdc++ is
doing anything wrong.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[glisse at gcc dot gnu.org]

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

--- Comment #6 from Marc Glisse <glisse at gcc dot gnu.org> ---
libc++ sfinae constrains std::swap.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[glisse at gcc dot gnu.org]

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

--- Comment #7 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #6)
> libc++ sfinae constrains std::swap.

More precisely: they do have swap declared before iter_swap, if you remove the
=delete it is ambiguous with libc++ as well, it just happens that the template
constraints disable the std swap in this particular case.

So I don't think libstdc++ is doing anything wrong.

I don't understand why ADL should get this to work. No function is more
specialized than the other, and ADL doesn't disable unqualified lookup. But
then again I am bad at understanding this text, it looks to me like it says ADL
shouldn't even happen, which is clearly not the case...

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[public at alisdairm dot net]

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

--- Comment #8 from Alisdair Meredith <public at alisdairm dot net> ---
I agree with Mark's analysis.  I was trying to force use of swap on a local
class, and found a pattern that worked for libc++ but missed that neither
template would be more specialized - my original attempt used friends ands not
valid for other reasons.  The key is that libc++ SFINAEs swap on the type
requirements, while libstdc++ defers the error until swap is instantiated.  I
am still surprised that I get an error for calling the move operations of
'local' though (in the second example) rather than getting an ambiguous lookup.
 I think the first example is unambiguously a user-error (mine) by this
analysis.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[glisse at gcc dot gnu.org]

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

--- Comment #9 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Alisdair Meredith from comment #8)
> I am still surprised that I get an error for calling the move
> operations of 'local' though (in the second example) rather than getting an
> ambiguous lookup.  I think the first example is unambiguously a user-error
> (mine) by this analysis.

The example with swap(T&,T&) is ambiguous. In the example with
swap(Swappable&,Swappable&), std::swap<local> is a better match (if it is not
removed by enable_if as in libc++) as it binds local& directly instead of a
reference to a base class.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Marc Glisse from comment #6)
> libc++ sfinae constrains std::swap.

Aha!  I suppose we could do that too, but that would be an enhancement (and
have to wait until after the 4.9 release), but apart from that I think we agree
now this is not a bug.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[public at alisdairm dot net]

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

--- Comment #11 from Alisdair Meredith <public at alisdairm dot net> ---
Created attachment 32298
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32298&action=edit
Portable test of ADL on local type

Agreed, not-a-bug.

For completeness, I attach a final test case that does perform ADL on a local
class to unambiguously find the right 'swap', properly using CRTP to inject the
friend that is the strongest match.  Thanks to David Rodriguez Ibeas for the
exact syntax to make this example work.

This example works correctly with both libstdc++ and libc++ - no bug.

Can I withdraw/close the issue myself?  (don't know gcc bug system for handling
user-error)

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, as reporter you can close it, the usual status for user error is
RESOLVED-INVALID, which I've now set it to.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[glisse at gcc dot gnu.org]

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

--- Comment #13 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #10)
> (In reply to Marc Glisse from comment #6)
> > libc++ sfinae constrains std::swap.
> 
> Aha!  I suppose we could do that too,

Indeed. I could never estimate the drawbacks properly. Doesn't it force types
to be complete earlier than it would otherwise? It probably isn't that bad if
libc++ got away with it.

[Bug libstdc++/60448] swap_ranges does not use ADL correctly

[public at alisdairm dot net]

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

--- Comment #14 from Alisdair Meredith <public at alisdairm dot net> ---
Among other things, it lets fools like me write code relying on a behavior, not
realizing that the code is not portable.  As a user I swing between the
convenience of having that utility, vs. the inconvenience of it being an
extension rather than mandated.  Other issues are whether rampant use of SFINAE
tricks has an impact on resource usage during compilation, and more confusing
error messages along the lines of "I could not find this function that you have
clearly supplied".  With Concepts Lite coming, there should be a cleaner way in
the language to get the same effects, so as a single user data point, I would
prefer to defer any non-standard-mandated changes along these lines until after
Concepts lands - in whatever form that turns out to be.  More research
demonstrating the cost is really not significant in medium-large scale software
might soften my view.