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

Adding value_to support without including a Boost.JSON header #550

Closed
pdimov opened this issue Apr 27, 2021 · 7 comments
Closed

Adding value_to support without including a Boost.JSON header #550

pdimov opened this issue Apr 27, 2021 · 7 comments
Assignees
Labels

Comments

@pdimov
Copy link
Member

pdimov commented Apr 27, 2021

Similar to #549, but in the other direction; it should be possible to add value_to support without including a Boost.JSON header. A minimal example is in https://godbolt.org/z/WsG33GrMP. Unlike the value_from case, this compiles, and the only reason it doesn't link is that the signature of value_to does not match the documented one.

I'm not sure why it's necessary for value_to to disappear from overload resolution when T is a reference. value_to is not intended to be an overload set. References can be disallowed by a static_assert in the body.

Not taking value in order to disallow implicit conversions is I suppose legitimate, but it can be achieved by adding a deleted templated overload (https://godbolt.org/z/Tqonvj9bT).

@vinniefalco
Copy link
Member

These declarations should have comments explaining the rationale for their signatures

@beached
Copy link

beached commented Apr 27, 2021

Have you considered not doing this and then they put their value_to into a header that includes their type and the boost.json header. That accomplishes the same thing of firewalling boost into a TU, no?

@pdimov
Copy link
Member Author

pdimov commented Apr 27, 2021

The easiest fix here is to change the specification of value_to as follows:

template<class T> T value_to( value const& v );
template<class T, class U> T value_to( U const& v ) = delete;

@EikeAtOT
Copy link

Unfortunately the changes in 31dd295 (see the above PR) make value_to available to global ADL. For our codebase that posed a breaking change causing ambiguous calls (we have a value_to in our own namespace). Before the above changes that problem did not occur.

@grisumbras
Copy link
Member

Can you provide a minimal example of what exactly was broken? We only have value_to overloads in namespace boost::json.

@EikeAtOT
Copy link

EikeAtOT commented Jul 28, 2022

#include <type_traits>
#include <iostream>

namespace A
{
    class Arg {};

    template<typename T, typename U = std::enable_if<std::is_same_v<T, Arg>, void>>
    void foo(const T&)
    {
        std::cout << "A::foo" << std::endl;
    }

    void bar(const Arg&)
    {
        std::cout << "A::bar" << std::endl;
    }
}

namespace B::detail
{
    
inline void foo(const A::Arg&) 
{
    std::cout << "B::detail::foo" << std::endl;
}

inline void bar(const A::Arg&)
{
    std::cout << "B::detail::bar" << std::endl;
}

int client()
{
    A::Arg x;
    foo(x);

    bar(x);
    return 0;
}

}

int main(int argc, char** argv)
{
    B::detail::client();
    return 0;
}

In client() the unqualified call to foo is well defined pointing to B::detail::foo while the unqualified call to bar is regarded ambiguous. The namespace A is basically boost::json. So it looks like when the argument is inferred through a template the overload resolution has lower precedence than the nearest overload in the current enclosing namespace.

@grisumbras
Copy link
Member

@EikeAtOT can you provide an example with value_to that has become broken after 31dd295, so that I can evaluate if this is something we can and should fix?

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

No branches or pull requests

5 participants