Old optional #67
Old optional #67
Conversation
drewdzzz
force-pushed
the
support_obj_in_optional
branch
from
1c3d6a7 to
7f8d4c4
Compare
The method is used to get the container which contains the value the path points to, so let's rename it to parent_container.
drewdzzz
force-pushed
the
support_obj_in_optional
branch
from
7f8d4c4 to
5976755
Compare
If MsgPack contains MP_NIL, the optional is reset. Otherwise, the underlying value is decoded just like without optional. The value is emplaced with default constructor if the optional is empty.
drewdzzz
force-pushed
the
support_obj_in_optional
branch
from
5976755 to
8a2048a
Compare
If passed optional is not empty, underlying value is encoded - optional itself is not present in the resulting MsgPack. If the optional is empty, it is encoded as MP_NIL.
drewdzzz
force-pushed
the
support_obj_in_optional
branch
from
8a2048a to
e24abec
Compare
There was a problem hiding this comment.
Looks good to me overall.
The only thing left unclear to me yet is how results are 'sinked' to destinations, and why container types are filled with 'contained' or, in your notion, 'transparent' values, e.g., int of std::optional<int> (or is that simply an optimization?).
You haven't mentioned this in your commit message, but as far as I understand, the trade-off under question is introducing an intermediate jump, i.e., jump_read_optional.
TBH, I didn't review the encoding test cases thoroughly, I think we need to setup a coverage workflow.
| { | ||
| if constexpr (TYPE == PIT_STATIC_L0) { | ||
| return std::get<POS>(std::tie(t...)).get(); | ||
| } else if constexpr (is_path_item_static(PI)) { | ||
| return tnt::get<POS>(prev(t...)); | ||
| return tnt::get<POS>(parent_container(t...)); |
There was a problem hiding this comment.
Nit: looks like this hunk belongs to first patch.
| @@ -645,8 +705,7 @@ struct JumpsBuilder { | |||
| if constexpr (path_item_type(LAST) == PIT_DYN_SKIP) { | |||
| return getFamiliesByRules<all_rules_t>(); | |||
| } else if constexpr (path_item_type(LAST) == PIT_DYN_KEY) { | |||
| using R = decltype(path_resolve(PATH{}, | |||
| std::declval<T>()...)); | |||
| using R = decltype(path_resolve(PATH{}, std::declval<T>()...)); | |||
There was a problem hiding this comment.
Nit: look's like an unnecessary hunk.
| @@ -659,8 +718,7 @@ struct JumpsBuilder { | |||
| return keyMapKeyFamiliesFlat<DST>(is{}); | |||
| } | |||
| } else { | |||
| using R = decltype(path_resolve(PATH{}, | |||
| std::declval<T>()...)); | |||
| using R = decltype(path_resolve(PATH{}, std::declval<T>()...)); | |||
There was a problem hiding this comment.
Nit: look's like an unnecessary hunk.
| mpp::encode(buf, mpp::as_arr(std::forward_as_tuple(0, 1, 2, 3, nullptr, 5))); | ||
| mpp::encode(buf, std::make_optional(mpp::as_arr( | ||
| std::forward_as_tuple(0, std::make_optional(1), 2, 3, std::optional<int>(), 5) | ||
| ))); |
There was a problem hiding this comment.
Nit: no sure it's worth it, but I think adding separate test cases would be cleaner than wrapping around existing ones.
Same comment for objects test case below.
| class PATH, class BUF, class... T> | ||
| bool jump_read_optional(BUF& buf, T... t) | ||
| { | ||
| auto&& optional_dst = unwrap(path_resolve(PATH{}, t...)); |
There was a problem hiding this comment.
Can we add some kind of assert about the type?
| @@ -1214,11 +1238,206 @@ test_object_codec() | |||
|
|
|||
| auto itr = buf.begin(); | |||
| mpp::decode(itr, rd); | |||
|
|
|||
There was a problem hiding this comment.
Nit: looks like an unnecessary hunk.
| fail_unless(rd == wr); | ||
|
|
||
| mpp::decode(itr, rds); | ||
| fail_unless(rds.count(wr) > 0); |
There was a problem hiding this comment.
How is this test case related to the patch?
| return Res::get(t...); | ||
| } | ||
|
|
||
| /** Resolve path, semi-transparent object are invisible. */ |
There was a problem hiding this comment.
I guess the comments should be the other way around, i.e., visible for path_resolve and invisible for path_resolve_transparent.
|
Another implementation of optional has been merged just now. |
The patch populates
mpp::encodeandmpp::decodefunctions with optional objects support. An empty optional is encoded asMP_NIL, andMP_NILis decoded as an empty optional. Otherwise, underlying objects are encoded/decoded just like without optional.