RSDK-6636 - refactor AttributeMap and ProtoType #221

stuqdog · 2024-03-13T14:40:31Z

Currently this is just changes to proto_type.hpp, still a WIP

Assuming we're happy with the shape of this, I will go about renaming the file and updating the rest of the code base.

acmorrow · 2024-03-19T18:14:09Z

src/viam/sdk/common/proto_type.hpp

-    }
+    class value_type;
+
+    using map_type = std::unordered_map<std::string, value_type>;


Unfortunately, my understanding is that unordered_map with an incomplete type has undefined behavior. It may happen to work depending on the implementation. For C++17, some standard container types were given defined behavior when used with incomplete types, but I think it is limited to std::vector, std::list, and std::forward_list:

https://stackoverflow.com/questions/41913578/how-can-i-declare-a-member-vector-of-the-same-class/41913654#41913654

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4371.html (C++17)

ack, will change to a unique_ptr

I don't think you need to change to unique ptr. You just can't declare this typedef with value_type as an incomplete type. But the recursive_wrapper stuff can handle it. Adding unique_ptr is just doubling up on the indirection that recursive_wrapper already provides.

acmorrow · 2024-03-19T18:26:51Z

src/viam/sdk/common/proto_type.hpp

+        using base_value_types_ =
+            boost::variant<boost::blank, std::string, const char*, int, float, double, bool>;
+        using value_types = boost::make_recursive_variant<
+            base_value_types_,


This doesn't look quite right to me. It looks like this is declaring a variant within a variant. Did you mean to use mpl and make_recursive_variant_over? Or does make_recursive_variant treat the first type specially in way I don't know about?

Hmm... I agree that seeing boost::variant and then boost::make_recursive_variant on it immediately after is a little strange. My thinking though was that we still wanted the root type to be a variant. So in essence the three cases for a value type are

| boost::variant<...>, | std::vector<boost::variant<...>>, | std::unordered_map<std::string, boost::variant<...>>,

In playing around with this definition I was able to build a value_type with raw values, with lists, with maps, with nested lists/maps, etc. etc. I also got the sense that make_recursive_variant_over was largely equivalent to make_recursive_variant, per description here.* That said, I can definitely believe I'm missing something and that use of mpl/make_recursive_variant_over would produce superior results!

type has behavior equivalent in every respect to [make_recursive_variant](https://www.boost.org/doc/libs/1_84_0/doc/html/boost/make_recursive_variant.html)< Sequence[0], Sequence[1], ... >::type (where Sequence[i] denotes the i-th element of Sequence), except that no upper limit is imposed on the number of types.

Yes, make_recursive_variant_over is equivalent. What I'm puzzled about is why there are two levels of variant, rather than just:

boost::make_recursive_variant<boost::blank, ..., bool, std::unordered_map<std::string, boost::recursive_variant_>, std::vector<boost::recursive_variant_>>

Why the second level of variant? I wouldn't expect it to be needed.

Ahh I see, that makes sense. Yeah I don't think the second level of variant is needed, will fix.

acmorrow · 2024-03-19T19:14:30Z

src/viam/sdk/common/proto_type.hpp

-};
-
-}  // namespace prototype_details
+}  // namespace value_type_details


The purpose of get_helper was to specialize for the AttributeMap case to deal with the extra layer of pointer with an automatic dereference in the populated case. Since the specialization has been removed, it suggests that get_helper is no longer needed. But I have a suspicion it still may need to exist, but with different specializations.

acmorrow · 2024-03-19T19:17:07Z

src/viam/sdk/common/proto_type.hpp

+        value_type(std::vector<value_type> v);
+        value_type(map_type);
+        value_type(google::protobuf::Value value);


For at least these types, copying is pretty expensive. I recommend providing both a const& and a && overload.

Alternatively, you could write a universally forwarding constructor:

template<typename T> value_type(T&& t)

However, that tends to be too greedy without some SFINAE to make sure it doesn't get invoked when it shouldn't.

acmorrow · 2024-03-19T19:21:24Z

src/viam/sdk/common/proto_type.hpp

+        value_type(double d);
+        value_type(bool b);
+        value_type(std::vector<value_type> v);
+        value_type(map_type);


This constructor is going to prove to be troublesome due to the containers vs incomplete types issue from above.

Ahh yeah, wrapping these in unique_ptr now.

That won't help though, because the container type still can't handle the forward declaration reliably.

I see, I was misunderstanding. Thanks for clarifying! If I put the using map_type = ... inside value_type class proper, is that allowed? I assume so but am tripping myself up a bit because at whatever point the using line exists, the class isn't fully defined at that point.

acmorrow · 2024-03-19T19:28:43Z

src/viam/sdk/common/proto_type.hpp

+       public:
+        template <typename T>
+        auto get() {
+            return value_type_details::get_helper<T>::get(value_);


Without any specialization for get_helper<T>, these just decay into boost::get. But I think you may want new specializations? If a user asks for the std::vector<value_type> member, does it get you it, or do you get the wrapper? I'm not actually sure.

acmorrow · 2024-03-19T19:30:23Z

src/viam/sdk/common/proto_type.hpp

+    // CR erodkin: consider how to return a ref or pointer to the value type such that we don't have
+    // to copy the `value_type`, while retaining guarantees of const-ness.
+    value_type get(const std::string& key) const;


Is there a problem with just having:

const value_type& get(...) const; value_type& get(...);

Hmm... I recall getting compiler warnings when I tried that before but it seems to be fine now so I must be misremembering! Yeah this looks good.

acmorrow · 2024-03-19T19:31:13Z

src/viam/sdk/common/proto_type.hpp

+
+    // CR erodkin: consider how to return a ref or pointer to the value type such that we don't have
+    // to copy the `value_type`, while retaining guarantees of const-ness.
+    value_type get(const std::string& key) const;


What value_type gets returned if the item is not found? Or does it throw?

Yeah good question. My intuition is returning a boost::blank as the default is best, people can check for the "not found" case pretty easily (if (!foo)) and it saves having to potentially large chunks of code in a try/catch.

acmorrow · 2024-03-19T19:31:40Z

src/viam/sdk/common/proto_type.hpp

+    // set a new k/v pair in the AttributeMap
+    void set(std::string key, value_type::value_types value) const;
+    void set(std::string key, value_type value) const;
+
+    // append a single value to a list type within the AttributeMap
+    void append(const std::string& key, value_type::value_types value);
+    void append(const std::string& key, value_type value);
+
+    // add a single key/value pair to a map type within the AttributeMap
+    void insert(const std::string& key, std::pair<std::string, value_type>);
+    void insert(const std::string& key, std::pair<std::string, value_type::value_types>);


It may just be better to expose the map_ rather than try to define your own wrapper above the unordered_map interface.

Yeah, I agree this is trying to do to much. I'm wondering though if it's better to keep the AttributeMap class around and expose map_ or to just define value_type at the top level and have AttributeMap just be a typedef on the map type. The latter seems more intuitive to me personally, though it does perhaps make get helpers slightly more awkward if they're just free standing functions rather than a class method.

prototype header

8736b56

stuqdog requested a review from acmorrow March 13, 2024 14:40

stuqdog requested a review from a team as a code owner March 13, 2024 14:40

stuqdog requested review from njooma and purplenicole730 and removed request for a team March 13, 2024 14:40

stuqdog marked this pull request as draft March 13, 2024 17:43

stuqdog marked this pull request as ready for review March 18, 2024 19:20

acmorrow requested changes Mar 19, 2024

View reviewed changes

stuqdog added 2 commits March 20, 2024 10:43

cr comments

4cae289

cr comments

58f49f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-6636 - refactor AttributeMap and ProtoType #221

RSDK-6636 - refactor AttributeMap and ProtoType #221

stuqdog commented Mar 13, 2024 •

edited

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

acmorrow Mar 20, 2024

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

acmorrow Mar 20, 2024

stuqdog Mar 20, 2024

acmorrow Mar 19, 2024

acmorrow Mar 19, 2024

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

acmorrow Mar 20, 2024

stuqdog Mar 20, 2024 •

edited

acmorrow Mar 19, 2024

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

acmorrow Mar 19, 2024

stuqdog Mar 20, 2024

RSDK-6636 - refactor AttributeMap and ProtoType #221

Are you sure you want to change the base?

RSDK-6636 - refactor AttributeMap and ProtoType #221

Conversation

stuqdog commented Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog commented Mar 13, 2024 •

edited

stuqdog Mar 20, 2024 •

edited