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

add pair/tuple conversions #624

Merged
merged 2 commits into from
Jun 19, 2017

Conversation

theodelrieu
Copy link
Contributor

I left a question in the code: Should we impose that the JSON array size must be equal to the number of tuple/pair elements?

If not, some elements in the JSON could be discarded:

json j = std::make_tuple(1, 2, 3, 4);
auto p = j.get<std::pair<int, int>>();

Some users might want to discard values on purpose, so I would say we should not check the size at all.

@nlohmann
Copy link
Owner

What would be the behavior if I try to convert [1] to a std::pair<int,int>?

@theodelrieu
Copy link
Contributor Author

It would throw, I use at in the implementation

@nlohmann
Copy link
Owner

Right, and it makes sense to complain about this (I guess it's the same behavior when you try to read four values from a three-value tuple?).

I think discarding values is OK, too, as long as we mention this in the documentation somewhere.

@theodelrieu
Copy link
Contributor Author

Yes you get a compile-time error if you go out of bounds with tuple.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 99.722% when pulling 80f701e on theodelrieu:feature/pair_tuple_conversions into 889006f on nlohmann:develop.

@nlohmann
Copy link
Owner

In #553, we excluded a conversion from json to std::array. If we have compile-time checks for tuples, maybe adding support for arrays in the same fashion is also possible?

src/json.hpp Outdated
template <typename BasicJsonType, typename... Args>
void from_json(const BasicJsonType& j, std::pair<Args...>& p)
{
// should we check that j.size() exactly matches the number of arguments for tuple and pairs?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's OK not to check this. The comment can be removed.

CHECK(j[3][0] == 0);
CHECK(j[3][1] == 1);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test case for the case when we have an array with, say, 3 elements and ask for a std::pair and something similar with tuples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is, sorry for the delay.

@theodelrieu theodelrieu force-pushed the feature/pair_tuple_conversions branch from 80f701e to 6e4910d Compare June 19, 2017 08:16
@theodelrieu
Copy link
Contributor Author

theodelrieu commented Jun 19, 2017

I will take a look at std::array right now.

Edit: It might be better to check the JSON array size, and throw a nice exception, instead of relying on at, what do you think?

Edit2: While we're at it, I think we should discuss about pre/postconditions of from/to_json methods provided by the library, and then add a section in the documentation.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.723% when pulling 6e4910d on theodelrieu:feature/pair_tuple_conversions into 112a6f4 on nlohmann:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.724% when pulling 08d7810 on theodelrieu:feature/pair_tuple_conversions into 112a6f4 on nlohmann:develop.

@nlohmann
Copy link
Owner

@theodelrieu Yes, a nice exception would definitely help. I guess most people would never expect at to be called in the first place.

And yes, the pre/post conditions should be documented. I think the README paragraph is the best place to start.

@nlohmann nlohmann merged commit 4e6f548 into nlohmann:develop Jun 19, 2017
@nlohmann
Copy link
Owner

Thanks a lot, @theodelrieu !

@theodelrieu theodelrieu deleted the feature/pair_tuple_conversions branch March 13, 2019 08:45
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

Successfully merging this pull request may close these issues.

3 participants