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

Separate container concepts #319 #327

Merged
merged 32 commits into from
Jul 2, 2023

Conversation

justend29
Copy link
Collaborator

  • commit 4dd573f contains exclusively autoformatting, and diffs can be made against that to see exclusively code changes
  • map_t has been split into readable_map_t and writable_map_t, as discussed in Separate concepts for reading and writing containers #319
  • the to_json<writable_map_t>::op() function needed to be slightly changed to depend less on the member functions of standard associative containers and instead operate a bit more generically. This allowed operating on input ranges
    • there was also what I think was a slight bug that was fixed in this rewrite
  • in the same line, T::value_type was replaced with range_value_t throughout the code

Note:
std::pairs are currently serialized as arrays of the form [first, second]. Existing code that serializes an array of pairs would expect [[first1,second1], [first2,second2],... ]
This PR changes that behaviour, as that array will now be considered an input range of pairs (map-like), and print them as objects. That is technically a breaking change.

It kind of seems more appropriate to serialize pairs as objects w/ a single entry of the form {"first": second}. We could lean into the breaking change and serialize single pairs as objects. May just be a preference, though.

@justend29
Copy link
Collaborator Author

@stephenberry I cannot immediately see why MSVC fails to compile from the error logs in CI. I'll run a VM to compile this locally with MSVC to hopefully understand why it's mad. Maybe this weekend - we'll see what time permits.

@stephenberry
Copy link
Owner

As a heads up, I'm working on fixes to MSVC on another pull request which should fix your builds here.

@stephenberry
Copy link
Owner

@justend29 MSVC 2022 now builds, and most of the 2019 errors have been fixed, but it seems to have problems with the ranges, and I think it's wrong. I'm really tempted to drop MSVC 2019 support, because there are so many hacks needed to get it to work and I'm starting to need to disable features for that compiler.

@justend29
Copy link
Collaborator Author

@stephenberry Thanks for those MSVC fixes. Is my understanding correct, that this PR will wait until either MSVC2019 support is dropped or more hacks and introduced to support it w/ the ranges introduced here?

Did you have thoughts about the changes themself - particularly, the "breaking" change and the handling of individual pairs?

@stephenberry
Copy link
Owner

@justend29 I intend to drop MSVC 2019 and merge this request. So, no need to fix the bugs.

I agree with you and I think std::pair should be interpreted as a key:value pair. A std::array<T, 2> or std::tuple of size 2 can be treated as a JSON array.

Can you update std::pair handling and add some unit tests for std::pair?

Changing:

auto num_view = std::views::iota(-2, 3) | std::views::transform([](const auto i) { return std::pair(i, i * i); });

to

auto num_view = std::views::iota(-2, 3) | std::views::transform([](const auto i) { return std::tuple(i, i * i); });

writes out the numbers as an array of arrays, which I think is great if the user wants that syntax. So I like how simple it is to get object syntax by just changing the code to use std::pair. Great work!

@justend29
Copy link
Collaborator Author

I'll absolutely update the handling of pairs with tests!
I'm glad you're happy with it :)

@justend29
Copy link
Collaborator Author

justend29 commented Jul 1, 2023

@stephenberry I've added reading & writing specializations for pairs, separating them from tuples.

I'll note that specializations for reading & writing binary were necessary in addition to the specializations for json. I am not familiar with glaze's binary mechanics, so I just used my judgment based on reading the code. I think you should review it thoroughly.

@stephenberry
Copy link
Owner

Thanks for all your additions. I'm going to merge. I looked over the binary handling and it looks good.

@stephenberry stephenberry merged commit 1589d3c into stephenberry:main Jul 2, 2023
12 checks passed
@justend29
Copy link
Collaborator Author

@stephenberry is that a typo at the end of the README?

@stephenberry
Copy link
Owner

@justend29, haha, thanks for pointing that out. Fixed.

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.

None yet

2 participants