Replies: 16 comments
-
I think this is a good way to alias keys. So, your issue seems to be with wanting to require all keys to be present. Do you mean that you want to require both |
Beta Was this translation helpful? Give feedback.
-
My definition of "all keys present" would be that either |
Beta Was this translation helpful? Give feedback.
-
I gotcha. Yeah, this isn't supported. It could be added, but it means more runtime logic for determining if the keys were all given, and so it doesn't seem like a generally good idea. Aliases usually make apis more difficult to deal with. And, what happens if we have three or four or five aliases to the same thing? I don't think it's a good idea to make sub maps within our primary map. I think it's best to just not require all the keys if you want aliases. If you need key requirement checks on sub objects and not globally for when there are not aliases, this is a feature that could be easily added without performance overhead. |
Beta Was this translation helpful? Give feedback.
-
I decided to take a look at Rust's It seems to use the actual name of the field as the 'primary' name, and then you can add aliases to the name on top of it. It then treats the aliases as if they were the same key, so if both are present it errors as if the key was duplicated. I haven't delved much into how glaze works, but I imagine at a high level there are a couple ways this could happen (ignore me if I make incorrect assumptions, this is just me hypothesizing):
|
Beta Was this translation helpful? Give feedback.
-
I have seen Rust's serde, but I'm not familiar with the codebase. I think the handling you described for aliases makes sense, and it could certainly be implemented. It also wouldn't affect the performance of anyone who doesn't use the feature, as glaze has compile time options. If it were implemented it would make sense to use sub (secondary) maps for aliases, however there are a number of concerns that we would need to think through in order to not make a bad implementation. Could you use a std::optional for values that you want to alias? This would allow you to check that the value was set (e.g. make it required while still aliased). This just wouldn't handle the case where the user duplicated the key by using the alias. |
Beta Was this translation helpful? Give feedback.
-
TLDR: I'd consider adding aliases support to the normal_and naive_map over the secondary map`` Implementation-wise for reading we don't need a secondary map except for handling required fields. However, performance wise the fastest thing to do but the most invasive change would be the second option @Ahajha proposed to add alias support naive_map and normal_map. Where "k" would do a lookup of the "key" iterator. This would impose zero additional runtime overhead. The maps are built compile time with perfect hashing and it's still just a hash -> reduction -> table_lookup for naive_map with or without aliases (normal_map also stays the same as always and just involves a conditional and potentially a secondary hash and reduction operation over naive_map). The index calculation for the required field bit_array would all be the same. The required fields bit mask and variant deduction might need to be updated though depending on how we implement this inside glz::object (If the alias information is part of the same item no updates need to be made and that's what id recommend anyways). For writing aliases would need to be skipped and this can be done compile-time so no overhead there either. And as long as they are combined into a single entry in the glaze object we don't even have to do that. I think aliases are common enough to prefer zero overhead approaches even if the implementation is more annoying. I'm more concerned with the user interface side of things. // 1. I would have loved it if we could do something like the following but it would be a breaking change
// I dont know if we can get the template deductions to work properly.
// We have CTAD for aggregates now but I think we need https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1167r0.html
glz::object({"key", &T::key, .comment="An old rusty key", .aliases={"k"}}, {"bob", &T::bob}});
// 2. The following would require allot of changes to the grouping code so I wouldn't recommend it
glz::object("key", &T::key, glaze::alias("k"));
// 3. This adds yet another special type to deal with but should only apply to the constexpr map building code. Also makes iterating over the object annoying (Filtering can be done compile time though).
glz::object(
"key", &T::key,
"k", glaze::alias("key")
);
//4. This is the least typing but is a bit more annoying implementation-wise for anything that iterates over the object like the write code (This is all compile time though).
// Its also annoying since it breaks all the code assuming the first item is the name and is convertable to string_view
glz::object(
glaze::alias{"key", "k}, &T::key,
); Not entirely happy with any of these to be honest but I thought id put ideas out there. Hopefully there is something better we can do. |
Beta Was this translation helpful? Give feedback.
-
Re: the backwards compatibility thought, you could use struct my_struct {
std::string key;
int bob;
struct glaze {
using T = my_struct;
static constexpr auto value = glz::fields{
{.name="key", .comment="An old rusty key", .aliases={"k"}},
{"bob", &T::bob},
};
};
}; Still probably a mess behind the scenes, but at least there's a way to avoid breakage. I'm terrified to look at the parsing stuff but I'd be willing to take a crack at the alternate syntax if it makes it easier to do aliasing. |
Beta Was this translation helpful? Give feedback.
-
Unfortunately I think we would require https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1167r0.html to get the deductions to work for that method. so it would have to be glz::fields(glz::field{.name="key", .comment="An old rusty key", .aliases={"k"}}) which is too much useless typing. I just want {"key", &T::key} most of the time and would like to just deduce everything. Its also annoying that c++ doesn't let you mix designated and non designated initializers like c does. c also has a bunch of other stuff like out of order, array, and nested designated initialization.
Provided @stephenberry agrees on a method for supplying the alias information I can deal with the implementation on the backend. I'm fairly familiar with all the areas that need to get changed and there should be zero additional runtime overhead. I think the most important thing is that aliases don't use a special way of supplying additional info and it can be extended to other pieces of metadata. That's why I would have liked what you posted if its feasible. We discussed this with comments and I cant remember what the plan was. |
Beta Was this translation helpful? Give feedback.
-
I played around with a few ideas, you're definitely right that if we were to go down my route it would require a bit more syntax. (https://godbolt.org/z/T4MzeTvxh for my half-baked experiment). Essentially the problem is that we require a "field" to have a templated type, and so it's impossible to make any kind of container for it, and so we'd have to make a variadic function That's a possible route, but in terms of end-user ergonomics the shortest syntax would probably be what you wrote with |
Beta Was this translation helpful? Give feedback.
-
Another problem that I'm not immediately sure how to solve is serializing. Currently the format can deserialize any combination of long and short names, and serialize to either short or long name (for the whole file). I'm not sure how we could specify that as an option. If abbreviations are a common use case, we could add an option as a special case of aliasing |
Beta Was this translation helpful? Give feedback.
-
I was hoping someone would figure it out my last two attempts ended in failure and I was hoping some of the newer CTAD stuff might make it possible.
I would just serialize to whatever the nonalias name is by default. People can make that the short name if they want to and use a long name as an alias so it is granular in that respect. We can add a compile-time option to serialize everything to the first shortest name ({"pie", "p", "pi", "pizza", "z", "za"} would be "p") since that would be the most common case for overriding default behavior. This can be determined compile-time so I don't think we need a special case for glz::abbr. Also, an option to serialize to the shortest name or to use abbreviations could be added later so Im not to worried until someone wants/needs that (Although the tagged binary format probably wants that). |
Beta Was this translation helpful? Give feedback.
-
The only downside of the |
Beta Was this translation helpful? Give feedback.
-
We don't necessarily need to add additional syntax and can detect when someone points to the same member twice but this doesn't work with stuff like lambda functions so Id rather not go that route. It also makes things confusing if multiple comments are supplied. |
Beta Was this translation helpful? Give feedback.
-
My favorite syntax is 4 from above: glz::object(
glz::alias{"key", "k"}, &T::key,
); This nicely groups all keys together and is clear for the user. I'm cool if you guys want to implement this approach. @mwalcott3, we should be able to just have a I think that when writing out the value, the first key listed should be written out. This way to user can explicitly decide what key they want to have written out. And, do we allow multiple aliases? I'm tempted to only allow one alias. This would simplify requirement checking and keep it more efficient. And, I don't really want to support practices of having a ton of aliases. |
Beta Was this translation helpful? Give feedback.
-
Yeah I'd be cool id be cool with implementing that. Helper function wise I think a get_names, get_name, get_aliases, and get_shortest_name would be good. 95% of the time we just the actual name and don't care about the aliases except when building the maps. If you want to restrict it to two I think that's fine. We can always increase it later. I'll make the maps support multiple aliases incase it needs to be expanded. Now that you brought up checking reqs there are some weird interactions between aliases and poly. I think we will skip having poly worry about aliases until someone wants it to support them. I don't see a strong use case for needing them there. |
Beta Was this translation helpful? Give feedback.
-
I have converting this into a discussion, as I feel aliasing keys and requiring them is useful in the long term, but right now it is not a priority and there are a number of optimizations to be made to key parsing that take precedence and would likely affect this implementation. |
Beta Was this translation helpful? Give feedback.
-
I have a format that currently supports long and short names (for example "k" and "key"), and they should map to the same value. In my prototyping to rewrite the parser, I have the following idiom:
This works as a good first step, but I would like it to require all keys to be present, and this would not work with that restriction.
Is aliasing already supported, or if not, would it be reasonable to add?
Beta Was this translation helpful? Give feedback.
All reactions