-
Notifications
You must be signed in to change notification settings - Fork 102
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
std::map with custom class as keys #507
Comments
This is currently the case but I don't believe it has to be. I'm looking into it. |
Pull request #509 introduces support for reading arbitrary types as JSON object keys. It also provides a fix for writing arbitrary types as JSON object keys. I'd like to get a review on it before merging. As for your example, it works with the PR but there are a few things to note:
Here is an altered version of your example: #include <cstdint>
#include <cassert>
#include <compare>
#include <glaze/core/meta.hpp>
#include <glaze/core/macros.hpp>
#include <glaze/json/quoted.hpp>
struct Foo {
int64_t value;
[[nodiscard]] std::strong_ordering operator<=>(const Foo&) const noexcept = default;
};
template<>
struct glz::meta<Foo> {
static constexpr auto value{ &Foo::value };
};
struct Bar {
std::map<Foo, int> values;
GLZ_LOCAL_META(Bar, values);
};
int main() {
const auto parsed = glz::read_json<Bar>(R"({"values":{"3":7,"7":3,"73":37}})");
assert(parsed.has_value());
const Bar expected{
.values = {{{3}, 7}, {{7}, 3}, {{73}, 37}}
};
assert(expected.values == parsed->values);
}
|
Hey, thank you very much for looking into this and for directly providing a pull request! I am not sufficiently accustomed with glaze to give a reasonable review on the internal changes, but I think that it would solve my problem and is everything I currently need. However I think it might makes sense to discuss the general serialization syntax, as raised in your second point (you are totally correct with your first point btw, my comparison operator must have gotten lost when creating the minimal example): You say that a
That being said, I am not sure if there is the best way to handle map-like types and it might makes sense to support several options. There also doesn't seem to be a consensus on this problem among other c++ JSON libraries, on how to handle map-like types with non-string keys. |
I'm really pleased that this will help resolve your problem. Addressing your points above:
Considering the idea of additionally allowing arrays of arrays may be worthwhile, possibly in the form of a modifier/wrapper, and could be useful for multi-maps. If a map isn't right for your use case, there are other options. you could look into an array type + |
Hey, thank you for your thoughts. My actual use case, which made me stumble over the inability to use arbitrary types as keys, is really rather simple. Think of All in all I think your patch will work fine for me. I just thought I'll raise this issue now, because once the patch is merged, changing the way maps with custom keys are serialized in the future would be a breaking change, while right now you can freely choose the format. So for me the following is just for the sake of argument. Feel free to ignore it if you have already made up your mind. (-:
|
That all seems understandable, and most of this debate seems to be coming from just looking at different sides of the same coin. Thanks for sharing the perspective. I think adding a modifier/wrapper to read/write object-like types as arrays could be in the cards, for sure. That doesn't break anything, is opt-in, and supports both trains of thought. |
This is a great discussion. In my thinking, pair and dictionary types should default to JSON objects, because we already have |
I agree completely. |
I believe an adaptor to provide an array-like interface to a Anyway, I'll look into it and file a PR |
@justend29 Definitely, provide an adapter (what we typically call in glaze a |
Thank you for your responses. I think that providing an adapter like Not sure if I understand the suggestion for a compile time option correctly. That would be a compile flag switching the format globally? I guess that I would personally stand to profit from such an option, as I obviously prefer the array format in most cases. However if there are those adapters I am not sure such option would even be necessary and worth the additional maintenance and testing needed to maintain such a global option. |
@Haatschii |
Ah yes, that makes more sense. Thanks for the explanation. |
The following minimal example fails to compile:
Following the error message it seems that the reason is a failing
static_assert
, which, as far as I understand, enforces that only string-like and arithmetic types can be used when reading astd::map
(I might be missing something, though).Is this indeed the case, that only string-like and arithmetic types are allowed as keys?
The text was updated successfully, but these errors were encountered: