RSDK-8541: ProtoValue get_unchecked and visit API #279

lia-viam · 2024-08-14T20:03:47Z

Adds an unchecked_get API to get the stored value in a ProtoValue, and a visit API in the spirit of std::visit or similar.

Tests both unchecked_get and visit. Note that the ProtoValue tests have been moved into their own source file now. In this diff all that's new in test_proto_value.cpp is the get_unchecked tests. The visit tests are in their own test source file.

…ure/proto-value-visit

acmorrow

LGTM, but please address the things I raised here before merging. If you want me to take another look before merging just re-request my review, otherwise, I don't think another round of review is required unless there are substantive changes required to address any of my comments. I don't think there will be.

acmorrow · 2024-08-30T17:25:29Z

src/viam/sdk/common/proto_value.hpp


 template <>
-struct kind_t<int> : std::integral_constant<int, 2> {};
+struct kind<int> {


It just occurred to me that proto's struct doesn't actually have an integer type. Just double. Do we want to be in the business of defining that conversion, or should we push that to users, since we cannot always know their intent.

I've been wondering about this too--proto just has a number type which promotes everything to double. There are some gotchas in the unit tests about this:
https://github.com/lia-viam/viam-cpp-sdk/blob/feature/proto-value-visit/src/viam/sdk/tests/test_proto_value.cpp#L43

Having mulled it over, I think it should not have an int case, or conversion functions from int. It gives the user the illusion that int is supported, but it really isn't, since it becomes a double. If anything, I'm almost tempted to say that we should =delete the ability to construct ProtoValue from integers, so that the caller must deal with the conversion themselves.

I'm in favor of getting rid of the separate int and double types and replacing them with a number type which is just a double underneath, as this would match what's in proto
https://protobuf.dev/reference/protobuf/google.protobuf/#value

Deleting the conversion from int entirely is tempting, but maybe this will cause us headaches when migrating from ProtoType? The variant underlying that class also has distinct int and double, which is arguably a mistake for the same reason, but there will definitely be instances of std::make_shared<ProtoType>(5)

OK, so several thoughts have come from this discussion:

Does ProtoValue really need a template constructor, or should it just have constructors for each type. As written, you will just get a funky probably-link-time error because it can't instantiate it if you try to, say, construct a ProtoValue of some unrelated type.

Should (some of the) ProtoValue constructors (templated or not) be explicit?

I suspect that if we eliminate the ability to hold integers, then the one numeric constructor should take double, but allow implicit conversions (so, not explicit). That'd let std::make_shared<ProtoType>(5) still work, I think. What I want to avoid is confusing situations where by accepting int as a constructor argument, we allow things with conversion to int to be passed, but then those things become doubles internally.

All good questions. My thoughts

I may as well just spell out a bunch of non-template constructors for each type, although I think I would still want to have them defer to a private template constructor. Whatever amount of duplication is present it would still be fewer lines than all the explicit instantiations I'm writing

I think yeah ideally we would want to follow the example of JSON libraries, where you have sort of easy ergonomic interfaces for building up objects like this, which you get because the constructors are not explicit.

Yeah I think this makes sense and we will just be deferring to standard C++ language rules about type promotion/argument overloading

All that said, maybe I should make a separate PR to do

have only number type

define non-template public constructors

make kind an enum and don't expose kind_t

and then rebase this one off that one?

Yes, let's go forward with what you have now where int is a thing, and then defer the whole int removal and any associated constructor fussing into a followup PR. I don't think you need to do the rebase part.

OK cool so just to confirm should I still do the kind_t removal and changing to an enum Kind for this PR too?

Yes, I think the kind_t removal is pretty trivial, and I'd really like to see the default case go away if possible, so I think the enum thing makes more sense in this review than in a new one.

acmorrow · 2024-08-30T17:26:05Z

src/viam/sdk/common/proto_value.hpp

+/// This type trait can be used to induce substitution failure on non-ProtoValue constructible
+/// types.
+template <typename T>
+using kind_t = typename proto_value_details::kind<T>::type;


Do you need the _t?

I suppose not, more just convention

Actually just tried doing this change, this made the compiler complain because of the ambiguity with the kind() method, I think we might be better off keeping the _t, or possibly using capital Kind, actually not super clear what our convention is here as I think it varies a bit throughout the SDK

Actually, do you even need kind_t? Is there a reason not to just say proto_value_details::kind<T>::type where you otherwise would have said kind_t? I don't see that kind_t figures in the public API of ProtoValue anywhere.

Good point!

src/viam/sdk/common/proto_value_visit.hpp

acmorrow · 2024-08-30T17:33:53Z

src/viam/sdk/common/proto_value_visit.hpp

+            return std::forward<Visitor>(visitor)(value.get_unchecked<std::vector<ProtoValue>>());
+        case kind_t<ProtoStruct>{}:
+            return std::forward<Visitor>(visitor)(value.get_unchecked<ProtoStruct>());
+        default:


Is a default needed at all? Should it result in calling the nullptr case for visitor? I feel like it shouldn't be possible or should be treated as some sort of fatal error.

It's "needed" just to not make the compiler complain, but in theory it's not possible by the invariants we maintain on the proto value objects. We could do a BOOST_UNREACHABLE_RETURN or similar.

Would having kind return an enum over the available types placate the compiler about not having a default?

I think that would work actually, I'll give it a shot

src/viam/sdk/tests/test_proto_value.cpp

src/viam/sdk/tests/test_proto_value_visit.cpp

src/viam/sdk/common/proto_value.cpp

…ure/proto-value-visit

lia-viam · 2024-09-16T16:30:56Z

@acmorrow just to sum up for re-review, addressed most of the comments, further decision probably needed on the "number" type and on if we want to put unreachable in our switch statements, which also affects existing code.

acmorrow

This looks great. I'm really happy with how little implementation lives in proto_value.hpp now, and the new visitation functions look great.

I think the only things left to do are:

See if it is possible to eliminate the default case
Decide about how to handle int (personal preference to disallow it and make the caller figure it out).

acmorrow · 2024-09-18T16:45:52Z

src/viam/sdk/common/proto_value.hpp

+/// This type trait can be used to induce substitution failure on non-ProtoValue constructible
+/// types.
+template <typename T>
+using kind_t = typename proto_value_details::kind<T>::type;


Actually, do you even need kind_t? Is there a reason not to just say proto_value_details::kind<T>::type where you otherwise would have said kind_t? I don't see that kind_t figures in the public API of ProtoValue anywhere.

acmorrow · 2024-09-18T16:51:49Z

src/viam/sdk/common/proto_value_visit.hpp

+            return std::forward<Visitor>(visitor)(value.get_unchecked<std::vector<ProtoValue>>());
+        case kind_t<ProtoStruct>{}:
+            return std::forward<Visitor>(visitor)(value.get_unchecked<ProtoStruct>());
+        default:


Would having kind return an enum over the available types placate the compiler about not having a default?

acmorrow · 2024-09-18T16:57:32Z

src/viam/sdk/common/proto_value.hpp


 template <>
-struct kind_t<int> : std::integral_constant<int, 2> {};
+struct kind<int> {


Having mulled it over, I think it should not have an int case, or conversion functions from int. It gives the user the illusion that int is supported, but it really isn't, since it becomes a double. If anything, I'm almost tempted to say that we should =delete the ability to construct ProtoValue from integers, so that the caller must deal with the conversion themselves.

acmorrow

This LGTM mod some suggested / minor tweaks to the new enum. No need to do another round of review for those changes unless you want to.

I like the new typedef for ListValue too.

acmorrow · 2024-09-20T17:16:13Z

src/viam/sdk/common/proto_value.hpp

 class ProtoValue {
   public:
+    /// @brief Type discriminator constants for possible values stored in a ProtoValue.
+    enum Kind { null, bool_, int_, double_, string, list, struct_ };


We usually prefix constants with k_ (google style thing, but one I like), so k_null, k_bool will all work I think.

I think maybe you should give these explicit numeric values since they form part of the interface.

Maybe make the order the same order as the struct proto does: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto#L66-L76. Throw int at the end since we expect to remove it.

Sounds good, going to do all these except keep the same order we have (but with int thrown at the end still). The reason for the proto sequencing escapes me, and I think it's easier mnemonically to write switch statements and sequence the methods if it goes from least to most data-rich types which is also what you see, eg, in JSON library APIs usually

lia-viam added 30 commits July 23, 2024 11:47

update paths to cpp files

cdbd2e0

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk

764cf2f

introduce proof of concept type erasure proto type

ce05487

add documentation and tests

3a24f90

nolint recursion

f72ebc1

make ctor helper an IILE and move the nolint

0759604

double nolint

a01ed14

add template keyword for pmf

7a41fa9

add helpers to simplify test case invocation

b834098

introduce coexisting pointer based converters

981becb

unfriend to_proto_value and remove model value version

4ae83d9

ABI insulate proto value

f391a30

add and test is_a and use static_cast instead of reinterpret_cast

79a4c4d

test copy construction and casting

afcdae7

add holder class and test more copy and move and null

ada9e37

add missing namespace

19159fe

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk

07b82ae

Merge branch 'main' into feature/type-erase-proto

41bd481

remove unneeded forward decl

4c8e592

update comments

9705aa2

use local stack storage for all types and test new move semantics

0e009aa

move small_size and remove unneeded default

8ab5d96

Merge branch 'main' into feature/type-erase-proto

184f11f

add struct_to_map and map_to_struct

a2b751a

use non ptr version

4e3d985

construct with move or rvalue

5b31f86

move list value

2483377

rename to vtable_t and remove noexcept in function pointers

26d7cc7

s/model_t/model/g

03bc715

s/storage_t/storage/g

b3e2f09

lia-viam added 5 commits August 14, 2024 15:44

make kind_t sfinae-able

95be658

remove unused include

5bba955

add and test proto value visit

e68a3bd

document visit api

cb35f2d

change include order

57553a7

lia-viam requested a review from a team as a code owner August 14, 2024 20:03

lia-viam requested review from njooma, stuqdog and acmorrow and removed request for a team August 14, 2024 20:03

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into feat…

df74f74

…ure/proto-value-visit

acmorrow approved these changes Aug 30, 2024

View reviewed changes

lia-viam added 5 commits September 16, 2024 12:00

code review: change include order

4b88dcf

code review: change include order

682a270

code review: change include order

1ab6ae5

put extern template in header

4e12704

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into feat…

c6fd7b2

…ure/proto-value-visit

lia-viam requested a review from acmorrow September 16, 2024 16:29

acmorrow requested changes Sep 18, 2024

View reviewed changes

lia-viam added 4 commits September 19, 2024 16:33

Merge branch 'main' into feature/proto-value-visit

47b70f9

replace integer types with enums and no longer expose kind_t

f55d8a7

remove switch defaults

3c330eb

alias protolist

31116da

lia-viam requested a review from acmorrow September 20, 2024 16:52

acmorrow reviewed Sep 20, 2024

View reviewed changes

acmorrow approved these changes Sep 20, 2024

View reviewed changes

use k_ for enum names and add explicit numerical values

f5e94cf

lia-viam merged commit 7d48705 into viamrobotics:main Sep 20, 2024
3 checks passed

lia-viam deleted the feature/proto-value-visit branch September 20, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-8541: ProtoValue get_unchecked and visit API #279

RSDK-8541: ProtoValue get_unchecked and visit API #279

lia-viam commented Aug 14, 2024

acmorrow left a comment

acmorrow Aug 30, 2024

lia-viam Sep 16, 2024

acmorrow Sep 18, 2024

lia-viam Sep 18, 2024

acmorrow Sep 18, 2024

lia-viam Sep 18, 2024 •

edited

Loading

acmorrow Sep 18, 2024

lia-viam Sep 18, 2024

acmorrow Sep 18, 2024

acmorrow Aug 30, 2024

lia-viam Sep 16, 2024

lia-viam Sep 16, 2024

acmorrow Sep 18, 2024

lia-viam Sep 18, 2024

acmorrow Aug 30, 2024

lia-viam Sep 16, 2024

acmorrow Sep 18, 2024

lia-viam Sep 18, 2024

lia-viam commented Sep 16, 2024

acmorrow left a comment

acmorrow Sep 18, 2024

acmorrow Sep 18, 2024

acmorrow Sep 18, 2024

acmorrow left a comment

acmorrow Sep 20, 2024

lia-viam Sep 20, 2024

RSDK-8541: ProtoValue get_unchecked and visit API #279

RSDK-8541: ProtoValue get_unchecked and visit API #279

Conversation

lia-viam commented Aug 14, 2024

acmorrow left a 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

lia-viam Sep 18, 2024 • edited Loading

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

Choose a reason for hiding this comment

lia-viam commented Sep 16, 2024

acmorrow left a 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

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lia-viam Sep 18, 2024 •

edited

Loading