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

Document when value_from is needed #845

Open
gummif opened this issue Jan 25, 2023 · 6 comments
Open

Document when value_from is needed #845

gummif opened this issue Jan 25, 2023 · 6 comments

Comments

@gummif
Copy link
Contributor

gummif commented Jan 25, 2023

Since 1.81 this no longer compiles:

struct customer
{
    int id;
    std::string name;
    std::vector<int> vec
};
void tag_invoke( value_from_tag, value& jv, customer const& c )
{
    jv = {
        { "id" , c.id },
        { "name", c.name },
        { "vec", c.vec }
    };
}

I see from #823 that this is intended. But it is not clear when calling value_from is needed and maybe the idiomatic way of doing it should be documented e.g. in the example here https://www.boost.org/doc/libs/1_81_0/libs/json/doc/html/json/quick_look.html. I assume the optimal way to do this now is the following

void tag_invoke( value_from_tag, value& jv, customer const& c )
{
    jv = {
        { "id" , c.id },
        { "name", c.name },
        { "vec", value_from(c.vec, jv.storage()) } // note pass storage to use the correct allocator
    };
}

Is this correct?

@grisumbras
Copy link
Member

First of all, yes, this code is correct.

Second, I honestly can't see the source of confusion: you need value_from when your type is not implicitly convertible to value. This is just like with any other type in C++. Is the problem with seemingly magical behaviour of std::initializer_list<value_ref> constructor/assignment?

@Triasmus
Copy link

Triasmus commented Apr 2, 2023

For me, I feel that it would make sense for boost::json to implicitly know to convert std::vector to a json array in this case. I just ran into this issue myself, where the implicit conversion was working fine in 1.80, and now I have build errors in 1.81 (I'm going back to 1.80 with the hope that I won't find the desire to use std::variant or std::optional).

So that's my source of confusion.
The quick look example page shows that value jv = {{"list", {1, 0, 2}}}; works. Replacing {1, 0, 2} with a pre-initialized vector<int> just makes sense.

@grisumbras
Copy link
Member

As, I've mentioned previosuly, the behaviour was never intended. If it was, json::value jv = std::vector{1,2,3} would also have been working, but it wasn't. The behaviour was a bug.

{{"list", {1, 0, 2}}}; does not mean any_map{ {"list", any_sequence{1, 0, 2}} };, it means json::object{ {"list, json::array{1, 0, 2} }.

I am not aware of any container type that does implicit conversions from other container types. In particular, standard container types definitely do not do that. There is a reason for that: implicit conversions cannot be locally disabled, they are pervasive, they create potential for lots of unintended conversions. They are a particularly terrible idea when memory is likely to be allocated.

@gummif
Copy link
Contributor Author

gummif commented Apr 9, 2023

I agree that this is a better design than implicit conversion. My heuristic is if an allocation is probably required then I need value_from and usually pass the storage ptr.

@rjahanbakhshi
Copy link

This could be related. In 1.80 the following used to work but in 1.81 it doesn't compile:

#include <boost/describe/enum.hpp>

enum class states
{
    failed,
    completed,
};
BOOST_DESCRIBE_ENUM(states, failed, completed);

#include <boost/json.hpp>

int main()
{
    boost::json::value value = {
        {"id", 42},
        {"state", states::failed},
    };
}

In 1.81 I need to use value_from otherwise it doesn't compile:

#include <boost/describe/enum.hpp>

enum class states
{
    failed,
    completed,
};
BOOST_DESCRIBE_ENUM(states, failed, completed);

#include <boost/json.hpp>

int main()
{
    boost::json::value value = {
        {"id", 42},
        {"state", boost::json::value_from(states::failed)},
    };
}

Of course, in 1.80 I had my own tag_invoke overload for described enum but it is added in 1.81.

It's a pity because the pretty initializer format is not working with 1.81+ for custom types or am I missing something?

@grisumbras
Copy link
Member

Yes, this behaviour was never intended and wasn't documented and so we removed the code that accidentally allowed it to work.

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

4 participants