[systems/lcm] Use shared_ptr for LCM serializers #19369
Conversation
jwnimmer-tri
force-pushed
the
rm-uniqueptr-lcmserializer
branch
from
0d69707 to
d535da8
Compare
jwnimmer-tri
added
release notes: newly deprecated
jwnimmer-tri
force-pushed
the
rm-uniqueptr-lcmserializer
branch
from
d535da8 to
8debfbe
Compare
jwnimmer-tri
force-pushed
the
rm-uniqueptr-lcmserializer
branch
from
8debfbe to
b81a91c
Compare
|
+@EricCousineau-TRI do you have time for feature review? (Or at least, design review?) |
There was a problem hiding this comment.
+@sherm1 for platform review, please!
Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
-- commits line 8 at r2:
BTW I'm not sure if it'd be completely useless, but yeah, I can't think of a workflow that would merit it for now.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW I'm not sure if it'd be completely useless, but yeah, I can't think of a workflow that would merit it for now.
I should amend the commit message when we merge.
My main reason to deprecate is that our "is cloneable" signature class Foo { virtual unique_ptr<Foo> Clone() = 0; } is extremely difficult to override correctly in Python, where unique ownership requires dangerous games with identifying shared_ptr use_count == 1 and then stealing the object.
So the reasoning for removal is more like "very little value + very high complexity".
There was a problem hiding this comment.
Platform
Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions
systems/lcm/serializer.h line 20 at r2 (raw file):
* %SerializerInterface translates between LCM message bytes and * drake::AbstractValue objects that contain LCM messages, e.g., a * Value<lcmt_drake_signal>. All `const` methods should be threadsafe.
minor: not clear how to interpret this sentence. Is it an instruction to developers about how to write const methods ("must be threadsafe") or a promise to users of the class ("const methods are threadsafe")? Please clarify.
For a given message type, it's OK to use one const serializer across all systems that publish or subscribe for efficiency. This also allows us to greatly simplify the Python bindings. Deprecate SerializerInterface::Clone. It now has very little upside, but is extremely difficult to override correctly in Python.
jwnimmer-tri
force-pushed
the
rm-uniqueptr-lcmserializer
branch
from
b81a91c to
c9b02b9
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions
systems/lcm/serializer.h line 20 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: not clear how to interpret this sentence. Is it an instruction to developers about how to write const methods ("must be threadsafe") or a promise to users of the class ("const methods are threadsafe")? Please clarify.
Hmm, it's both. I tried some working that is hopefully better. WDYT?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion
systems/lcm/serializer.h line 20 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Hmm, it's both. I tried some working that is hopefully better. WDYT?
This is good, thanks. The contract term for the user is all you need -- developers are automatically bound to deliver it.
Towards #19250. Undoes #15394.
This change is