Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RSDK-8541: ProtoValue get_unchecked and visit API #279
RSDK-8541: ProtoValue get_unchecked and visit API #279
Changes from 64 commits
cdbd2e0
764cf2f
ce05487
3a24f90
f72ebc1
0759604
a01ed14
7a41fa9
b834098
981becb
4ae83d9
f391a30
79a4c4d
afcdae7
ada9e37
19159fe
07b82ae
41bd481
4c8e592
9705aa2
0e009aa
8ab5d96
184f11f
a2b751a
4e3d985
5b31f86
2483377
26d7cc7
03bc715
b3e2f09
1f45217
c600450
dfa7575
1f07ecc
a333406
47bdd17
b0b0e15
875d968
4b01a12
ed70190
2a8cb3a
51e2b9a
7ca50e6
83de916
1516e81
b659c76
c549e8d
d5dc9cb
fb48286
50338ab
955eae4
cbcdc4e
fcf88ac
896480b
0536359
c3b27d1
f10ec1f
e94a94b
95be658
5bba955
e68a3bd
cb35f2d
57553a7
df74f74
4b88dcf
682a270
1ab6ae5
4e12704
c6fd7b2
47b70f9
f55d8a7
3c330eb
31116da
f5e94cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having mulled it over, I think it should not have an
int
case, or conversion functions from int. It gives the user the illusion thatint
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 constructProtoValue
from integers, so that the caller must deal with the conversion themselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of getting rid of the separate
int
anddouble
types and replacing them with anumber
type which is just adouble
underneath, as this would match what's in protohttps://protobuf.dev/reference/protobuf/google.protobuf/#value
Deleting the conversion from
int
entirely is tempting, but maybe this will cause us headaches when migrating fromProtoType
? Thevariant
underlying that class also has distinctint
anddouble
, which is arguably a mistake for the same reason, but there will definitely be instances ofstd::make_shared<ProtoType>(5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so several thoughts have come from this discussion:
Does
ProtoValue
really need atemplate
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 aProtoValue
of some unrelated type.Should (some of the)
ProtoValue
constructors (templated or not) beexplicit
?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 letstd::make_shared<ProtoType>(5)
still work, I think. What I want to avoid is confusing situations where by acceptingint
as a constructor argument, we allow things with conversion toint
to be passed, but then those things become doubles internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good questions. My thoughts
template
constructors for each type, although I think I would still want to have them defer to a privatetemplate
constructor. Whatever amount of duplication is present it would still be fewer lines than all the explicit instantiations I'm writingexplicit
.All that said, maybe I should make a separate PR to do
number
typetemplate
public constructorskind
an enum and don't exposekind_t
and then rebase this one off that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's go forward with what you have now where
int
is a thing, and then defer the wholeint
removal and any associated constructor fussing into a followup PR. I don't think you need to do the rebase part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK cool so just to confirm should I still do the
kind_t
removal and changing to anenum Kind
for this PR too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the
kind_t
removal is pretty trivial, and I'd really like to see thedefault
case go away if possible, so I think the enum thing makes more sense in this review than in a new one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the
_t
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose not, more just convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 capitalKind
, actually not super clear what our convention is here as I think it varies a bit throughout the SDKThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, do you even need
kind_t
? Is there a reason not to just sayproto_value_details::kind<T>::type
where you otherwise would have saidkind_t
? I don't see thatkind_t
figures in the public API ofProtoValue
anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would having
kind
return an enum over the available types placate the compiler about not having a default?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would work actually, I'll give it a shot