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

call to 'advance' is ambiguous #43

Closed
wickedmic opened this Issue Aug 31, 2018 · 16 comments

Comments

Projects
None yet
5 participants
@wickedmic

wickedmic commented Aug 31, 2018

I upgraded my boost library from 1.62 to 1.67 (issue remains with 1.68) and now I'm getting "call to 'advance' is ambiguous". After some investigation it looks like a using statment in boost/iterator/advance.hpp:80 is causing the issue. The problem arises if advance is called unqualified, so that ADL triggers. boost 1.65 seems to be the first release with this issue.

In my case this means that I cannot use range-v3 (this lib is making the unqualified call to advance) together with boost geometry (this uses boost-iterator's advance function and therefor includes boost/iterator/advance.hpp).

Here is a minimal code example, which causes this issue:
https://godbolt.org/z/rSfeVi

@Lastique

This comment has been minimized.

Member

Lastique commented Aug 31, 2018

I don't think the test case is correct. You can't expect unqualified advance to resolve to std::advance because the type of the iterator is unspecified (i.e. its type is not required to be in namespace std).

I think the right thing to do is to fix user's code (or range-v3) so that it doesn't call unqualified advance.

@ericniebler

This comment has been minimized.

Member

ericniebler commented Sep 19, 2018

I think the right thing to do is to fix user's code (or range-v3)

I'll note here that the test case has no range-v3 code in it. The right thing to do would be for Boost to not add an overload of advance that creates an ambiguity with the one in namespace std, and locate it such that ADL is likely to find it. One possible solution would be to turn boost::advance into a customization point object.

@Lastique

This comment has been minimized.

Member

Lastique commented Sep 19, 2018

The right thing to do would be for Boost to not add an overload of advance that creates an ambiguity with the one in namespace std, and locate it such that ADL is likely to find it.

advance in Boost.Iterator is already protected from inadvertent ADL. It's defined in boost::iterators::advance_adl_barrier. boost::advance and boost::iterators::advance are the names for direct public use.

@ericniebler

This comment has been minimized.

Member

ericniebler commented Sep 19, 2018

I don't think you're really understanding the issue. As @wickedmic pointed out above, the version of advance in the ADL-protecting namespace is brought into the boost:: namespace with a using declaration (which rather defeats the purpose of the ADL-protecting namespace) here:

using iterators::advance;

So an unqualified call to advance will find both the versions in boost:: and in std:: if the iterator type has boost and std as associated namespaces. That is not an uncommon scenario, I would imagine, especially for users of Boost.Iterator.

The way to make this work is to turn boost::advance into a customization point object.

@Lastique

This comment has been minimized.

Member

Lastique commented Sep 19, 2018

Sorry, I was thinking that ADL does not consider using-declarations, but apparently it only doesn't consider using-directives.

Making boost::advance a customization point object would solve the ambiguity but I'm not sure making it a customization point is what we really want. I don't think we want to dispatch between different implementations of advance. Perhaps just converting that using-declaration to a using-directive would suffice.

@morinmorin

This comment has been minimized.

Member

morinmorin commented Sep 22, 2018

IIRC, Boost.Iterator has never advertised boost::advance/distance.
The documentation only says boost::iterators::advance/distance.
So it might be OK to remove using iterators::advance/distance;
(and submit PR's to users of boost::advance/distance).
But this makes a discrepancy between Boost.Iterator and Boost.Range;
Boost.Range has boost::distance, but Boost.Iterator doesn't have it.

Perhaps just converting that using-declaration to a using-directive would suffice.

+1 and #44.

@Lastique Lastique closed this in b844c8d Sep 22, 2018

@morinmorin

This comment has been minimized.

Member

morinmorin commented Sep 22, 2018

Thanks Michael and Eric for reporting and pointing out the problem.
Also, thanks Andrey for merging the PR (and sorry for forgetting rebase & squash).

@Lastique

This comment has been minimized.

Member

Lastique commented Sep 22, 2018

I had to revert the change as it broke building Boost.

@Lastique Lastique reopened this Sep 22, 2018

@morinmorin

This comment has been minimized.

Member

morinmorin commented Sep 22, 2018

It seems that Boost.Range's boost::distance(rng) hides Boost.Iterator's boost::distance(it1, it2), if we use using-directives. So, boost::distance(it1, it2) cannot be found when <boost/range/distance.hpp> is included.

This sample code fails to compile:

namespace NS1
{
    namespace NS2
    {
        template <typename T, typename U>
        void f(T, U) {}
    }
    
    using namespace NS2; // Error
    // using NS2::f; // OK (but using-declarations have ADL issue)
    
    template <typename T>
    void f(T) {}
}

int main(int argc, char* argv[])
{
    NS1::f(1, 2);
    
    return 0;
}
@pdimov

This comment has been minimized.

Contributor

pdimov commented Sep 22, 2018

Iterator's boost::advance should be enable_if-ed on the category being one of Iterator's. (An object won't work - can't overload an object with Range's function.)

@pdimov

This comment has been minimized.

Contributor

pdimov commented Sep 22, 2018

Looking at

is_convertible<Cat,incrementable_traversal_tag>
, the right condition seems to be is_convertible<Cat, incrementable_traversal_tag>.

@pdimov

This comment has been minimized.

Contributor

pdimov commented Sep 22, 2018

... and it seems that we'll also have to using std::advance; in boost:: to support boost::advance for normal iterators.

Tricky.

@morinmorin

This comment has been minimized.

Member

morinmorin commented Sep 22, 2018

A possible fix:
Define Boost.Range's distance(rng) in ADL barrier namespace (currently, it is defined in boost namespace) and pull the name by a using-directive. Note that documentation of Boost.Range explicitly forbids unqualified call to distance(rng). So this modification to Boost.Range should be OK.

This code compiles fine:

namespace NS1
{
    namespace NS2
    {
        template <typename T, typename U>
        void f(T, U) {}
    }
    using namespace NS2;
    
    namespace NS3
    {
        template <typename T>
        void f(T) {}
    }
    using namespace NS3;
}

int main(int argc, char* argv[])
{
    NS1::f(1, 2);
    NS1::f(1);
    
    return 0;
}
@morinmorin

This comment has been minimized.

Member

morinmorin commented Sep 23, 2018

Regarding constraints on the first template parameter of advance: if ADL issues get resolved, I think "no constraints" would be OK (unless other libraries define boost::advance with two unconstrained template parameters).

@pdimov

This comment has been minimized.

Contributor

pdimov commented Sep 23, 2018

Yes, I agree.

@ericniebler

This comment has been minimized.

Member

ericniebler commented Sep 23, 2018

Nice. Thanks guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment